Add a skipCopy option to tracked-collections
#1131
Conversation
|
|
||
| The classes that implement the tracked-collections have an `options` argument. We can add an option to it to skip copying the collection argument that is passed. | ||
|
|
||
| The naming is of course the biggest issue. One might argue that it's good to have "unsafe" in the name because that really is unsafe - if the original collection is modified and it is not copied - that change will not be reflected in the template which might be confusing for the user. On the other hand, this is somewhat "obvious" so naming it "unsafe" might prevent people from using it and therefore skipping on potential (big) performance gains. |
There was a problem hiding this comment.
Yeah, i was thinking we go even shorter and copy: false
There was a problem hiding this comment.
I'm fine with any name. Let me know if I should update the RFC text with some specific name.
There was a problem hiding this comment.
Examples I could think of:
trackedArray(original, { copy: false }); // from this RFC
trackedArray(original, { copySource: false });
trackedArray(original, { copyUpstream: false });
trackedArray(original, { avoidCopy: true });
trackedArray(original, { skipCopy: true }); // from this RFC
trackedArray(original, { noCopy: true });
There was a problem hiding this comment.
Thanks, I added these to the RFC text.
|
|
||
| The classes that implement the tracked-collections have an `options` argument. We can add an option to it to skip copying the collection argument that is passed. | ||
|
|
||
| The naming is of course the biggest issue. One might argue that it's good to have "unsafe" in the name because that really is unsafe - if the original collection is modified and it is not copied - that change will not be reflected in the template which might be confusing for the user. On the other hand, this is somewhat "obvious" so naming it "unsafe" might prevent people from using it and therefore skipping on potential (big) performance gains. |
There was a problem hiding this comment.
naming it "unsafe" might prevent people from using it
I do believe that as well. I am afraid people might see this as "an option that should not be used, unless extreme situation", and might even use it incorrectly.
|
|
||
| ## How we teach this | ||
|
|
||
| We add it to the documentation. And also to the TypeScript types (which is already done in the PR above). |
There was a problem hiding this comment.
This section should contain the prose to teach the feature as well as where in the existing guides the feature should be discussed. It should also point out any places in the guides, tutorial or api docs that will need updating.
There was a problem hiding this comment.
I'm not sure the tracked-collections are described in the guides. They are rather new (at least them being built-in in Glimmer - the tracked-toolbox package has been around for a while). Or perhaps I just missed them - I don't see them in the tracked section and searching for trackedArray yields no results. Do you mind pointing me in the right direction if I've missed something?
There was a problem hiding this comment.
The code to build them in just landed on main so they will be in the API guides in a version or two.
They do appear in places in the guides, https://guides.emberjs.com/release/in-depth-topics/autotracking-in-depth/ is the tracking-focused page
There was a problem hiding this comment.
Adding the prose here with use-cases helps explain what is being proposed and why. It also prevents RFCs from becoming "stuck" after implementation when we have not yet created docs.
There was a problem hiding this comment.
@kategengler I added some example text to the RFC. Please let me know if that's what you expect.
Propose adding a
skipCopyoption to tracked-collectionsRendered
Summary
This pull request is proposing a new RFC.
To succeed, it will need to pass into the Exploring Stage, followed by the Accepted Stage.
A Proposed or Exploring RFC may also move to the Closed Stage if it is withdrawn by the author or if it is rejected by the Ember team. This requires an "FCP to Close" period.
An FCP is required before merging this PR to advance to Accepted.
Upon merging this PR, automation will open a draft PR for this RFC to move to the Ready for Released Stage.
Exploring Stage Description
This stage is entered when the Ember team believes the concept described in the RFC should be pursued, but the RFC may still need some more work, discussion, answers to open questions, and/or a champion before it can move to the next stage.
An RFC is moved into Exploring with consensus of the relevant teams. The relevant team expects to spend time helping to refine the proposal. The RFC remains a PR and will have an
Exploringlabel applied.An Exploring RFC that is successfully completed can move to Accepted with an FCP is required as in the existing process. It may also be moved to Closed with an FCP.
Accepted Stage Description
To move into the "accepted stage" the RFC must have complete prose and have successfully passed through an "FCP to Accept" period in which the community has weighed in and consensus has been achieved on the direction. The relevant teams believe that the proposal is well-specified and ready for implementation. The RFC has a champion within one of the relevant teams.
If there are unanswered questions, we have outlined them and expect that they will be answered before Ready for Release.
When the RFC is accepted, the PR will be merged, and automation will open a new PR to move the RFC to the Ready for Release stage. That PR should be used to track implementation progress and gain consensus to move to the next stage.
Checklist to move to Exploring
S-Proposedis removed from the PR and the labelS-Exploringis added.Checklist to move to Accepted
Final Comment Periodlabel has been added to start the FCP