Ember.A should not modify underlying array - #20245
Conversation
|
|
||
| const emberArrayLastObject = nonEnumerableComputed(function () { | ||
| return objectAt(this, this.length - 1); | ||
| }).readOnly(); |
There was a problem hiding this comment.
I don't love how we're assigning them to variables here, but I can't find any other good way to get a reference to their CPs.
| let proxy = new Proxy(arr, { | ||
| get(target, prop /*, _receiver */) { | ||
| // These nonEnumerableComputed properties need special handling | ||
| if (prop === '[]') { |
There was a problem hiding this comment.
Ideally we'd have something more generic, but we aren't going to be adding stuff to EmberArray going forwards so this should be fine.
|
@wagenet note that "Extend Prototypes" is failing on CI. |
7dd16a6 to
7aa79fa
Compare
chriskrycho
left a comment
There was a problem hiding this comment.
This seems reasonable to me, but I'd still really like it if someone who's more familiar with this could give it some 👀 as well. @rwjblue do you have 10 minutes (tops) to look at it?
|
I think I'd like to put this behind a flag. I think (hope) it won't cause issues for people, but who knows what people are actually doing and whether they're relying on the mutating behavior. |
7aa79fa to
0b2ab6f
Compare
00ccece to
582831f
Compare
|
I agree that it should go behind a flag. Because many addons call |
582831f to
331a4a6
Compare
|
@wagenet I'm working on pushing |
|
@locks This was partially motivated by issues Ember Data was having. @runspired what's the status of that stuff? |
|
still needed. iirc the solution for most has been to turn prototype extensions back on |
|
Is this still relevant? |
|
@kategengler I believe so |
|
@wagenet should we merge then? |
|
@kategengler I would like to and it is flagged so it should be safe. That said, I haven't really gotten a proper review from anyone still. |
I don't have a clue here or I would offer you one :) I'm just triaging PRs. |
|
can this be rebasod? |
331a4a6 to
dd75266
Compare
| // SAFETY: This will return an NativeArray but TS can't infer that. | ||
| return NativeArray.apply(arr ?? []) as NativeArray<T>; | ||
| if (EMBER_A_NON_MODIFYING) { | ||
| // Remove this in Ember 5.0. |
There was a problem hiding this comment.
Heh... should be 6.0 now, I guess.
|
Since we're planning to deprecate Ember Array and have not yet fixed this, should this be closed? |
|
Closing since we plan to deprecate Ember Array |
Resolves #20219