RFC: Deprecate array prototype extensions

[smilland]

Rendered text

[chriskrycho]

Thanks so much for doing this work! I will bring it to the Framework meeting this week for initial discussion. 💙

[bertdeblock]

Should we also display a deprecation warning at build time when EXTEND_PROTOTYPES or EXTEND_PROTOTYPES.Array is not set to false?

[smilland]

Should we also display a deprecation warning at build time when EXTEND_PROTOTYPES or EXTEND_PROTOTYPES.Array is not set to false?

That's a good point!

[runspired]

these false positives will be greatly reduced soon because in ember-data 4.8 usages of these has already been deprecated. This means remaining usage is from A() which I would hope this RFC would consider simultaneously deprecating or various other ArrayProxy implementations which are far less common.

[chriskrycho]

Agreed! One of the things I plan to discuss at Friday's Framework Core meeting is exactly whether we should deprecate @ember/array entirely and/or at least extracting it and move it out to a separate package which is not installed by default.

[runspired]

Sure, but the reason I mention A is that A mutates array instances when prototype extensions are turned off. If we're going to rip off the bandaide it seems better to rip it fully off vs allowing people to just call A on everything to get back to the same state.

[smilland]

Another sets of false positives are self-defined class functions, for exampleinvoke, clear are pretty common method names. ☹️

[chriskrycho]

@runspired yeah, I spent literally my whole day today dealing specifically with the horror caused by the NativeArray shenanigans as relates to types. If it were my preference alone, we'd deprecate it and cut a special v5.0 tomorrow just to get rid of A(). 😂

@smilland yeah, unfortunately that kind of thing is basically impossible to avoid 100% without full TS coverage (which we obviously can't require people to have!).

[ef4]

Should we also display a deprecation warning at build time when EXTEND_PROTOTYPES or EXTEND_PROTOTYPES.Array is not set to false?

I would like to suggest going even further than this and considering using only a single deprecation message for having EXTEND_PROTOTYPES in the wrong mode.

The reason I'd suggest this is that it's much easier to implement and I think it would actually give clearer feedback to developers. The instructions for clearing the deprecation would be to flip the flag and fix what breaks. The stack traces you'll get from the lack of prototype extensions will point at precisely the right place to make the fixes.

[chriskrycho]

We had three notes coming out of today's Framework Core discussion of this RFC.

1. @ef4's suggestion should dramatically simplify what we need to do from a tooling perspective to implement the RFCs. (The actual deprecation text you built for the deprecations app will remain applicable and useful, though!)

2. We should include in this RFC that we will also immediately switch the app blueprint to turn off the prototype extensions by default for new apps, so that we do not add unnecessary work to the plates of authors of newly-generated apps.

3. We would like to separately have a different RFC which is related to this one but which does not block this one to deprecate the @ember/array package which is part of ember-source, targeting Ember v5.0.

   The basic outline for that would be:

   • Extract @ember/array into a separate package.
     • Make it a hard error to have that package installed in an app using ember-source@4.
     • Document that part of the process of upgrading to ember-source@5 is to do one of the following:
       • Stop using @ember/array entirely.
       • Install the new extracted package manually.
   • Document that the new @ember/array package will work until whatever point the Ember Classic Object model is removed.

   This approach mirrors how we handled jQuery and Octane, with the @ember/jquery package, and tries to avoid the problems we had and have with the @ember/string package.

   @smilland if you’re interested in writing that RFC, it should be relatively small now that all the effort for this RFC is done—but you’ve already done a ton of work, so we understand if you don’t want to pick that up. Just let us know either way; if you aren’t interested, we’ll find someone else to pick it up!

   This RFC can become Accepted independent of that one, but that RFC and this one will move to later stages (probably Released and definitely Recommended) together.

Once (1) and (2) here are done, I think we have a clear path to moving this forward! Thanks again!

[runspired]

I know it feels out of scope but I want to reiterate that I feel we should more broadly deprecate A as part of this RFC, and/or we should intentionally change how A's behavior works.

The reason this concerns me so much is two-fold.

1. I think a lot of apps will end up using A as a crutch, they'll start defensively calling A on any array so that they don't end up with errors.

2. I don't believe that developers today understand what A does, and the significance of changing that flag.

• To recap the behavior of A, if Prototype extensions is "on" then A is a no-op. It returns to you exactly the same reference you passed in entirely unmodified.
• If Prototype extensions if "off" then it will clobber a large number of methods on the array or array-like instance, but still return to you the original ref.

I think the big problems are that it creates the potential for race-conditions in code that is otherwise not-aware of this instance conversion occurring, as well as it subtly changes the behavior of array-like in an extremely significant way: all array-like primitives will have to be careful to guard against their methods being clobbered as they are now below in the chain instead of above in the super chain. What's more, it takes something that folks don't understand today (namely that A(array) === array) and makes it all the more confusing.

I wonder if we could get away with A returning a new reference when prototype extensions are turned off instead of clobbering? Since folks are forced to refactor these call-sites anyway this would provide increased clarity, correctness and resilience around this primitive and prevent folks having a mysteriously hard time upgrading due to needing to find all the locations the same array reference had accidentally become dependent on A having been called somewhere else.

[runspired]

I also think that regardless of whether we add deprecation of or alteration of A to this RFC that we need to add support for computed chains to TrackedArray from TrackedBuiltIns, otherwise folks will feel forced to use A for arrays still to resolve this deprecation due to the thread-pull that refactor could entail vs refactoring to something more modern.

[chriskrycho]

@runspired that's an important call-out, which I'll make sure we address explicitly before putting this into FCP (hopefully in two weeks—I’m out next week). I think we should probably tackle that in the associated RFC instead of this one—we agreed already today that both need to go, but probably separately.

[bertdeblock]

Should we also include something about the ember-disable-prototype-extensions addon?
Does it remain in the addon blueprint output if we set EXTEND_PROTOTYPES to false?

[ef4]

We're putting this into final comment period because there seems to be consensus on the core proposal on how to remove the prototype extensions.

Everybody also agrees we should deprecate Ember.Array too, but it's going to be harder to do inside this RFC and should be its own RFC. In particular, there are interoperability concerns with computed properties that makes it nontrivial for people to refactor from A to TrackedArray, so there is work to do on smoothing that upgrade path.

[jrjohnson]

I spent some time removing these from our 7 year old Ember codebase last week and wanted to add a few notes from that process:

First: pushObject and pushObjects are the most difficult to remove as there are several gotchas.

1. It seems like pushObject was able to take an Array or maybe an ArrayProxy and split it and push the pieces. I would have assumed this would be the domain of pushObjects, but it seemed to work. Or possibly our code has always been a little bit broken and this change exposed something. Either way this takes some serious manual attention to resolve.
2. Updating Ember Data @hasMany relationships still requires the pushObject(s), this may be fixed in a more recent version, I'm not sure. But it means a global search/replace isn't an option.

Second: Sorting is hard. I don't think anyone will be surprised by this, but the code ember has for comparing values is well tested and has worked for us for a long time. I ended up copying compare, spaceship, and sortBy into a utility to do sorting in the exact way ember does it. I think it's probably worth either making sortBy public or pulling it into an Addon as it is otherwise super easy to break existing apps by not handling the variety of edge cases that are currently covered. Pulling this from lodash or somewhere else may be OK, I didn't do any kind of detailed comparison, but I wasn't willing to take a risk in breaking something by changing the complex core sort behavior we've relied on for a long time.

Lastly, as @runspired mentioned Ember Data 4.8 is going to handle it's array-like stuff better, in fact the deprecations of methods in ED (#745) are what caused me to get into this in our app and just replace them everywhere. However, until #846 is fully realized (and maybe some other ED RFCs) you're going to get a mix of types coming from resolving models and their relationships and passing those values around. Some of these types itteratable and some are not. I found myself pretty constantly messing with adding .slice() and removing it from various thing because I was wrong about the source of that data and would get errors from return [...posts, newPost] type code. I think it may make more sense to land the ED changes in 5.0 and then schedule this RFCs implementation to Ember in 6.0. Given the new more frequent major release process this hopefully isn't too far away, but will probably make for a much smoother transition for older apps like ours.

[robbytx]

The biggest downside of this change for me is the loss of the .findBy, .filterBy, .mapBy (and to a lesser extent .sortBy and .rejectBy). I use these heavily through my apps, as they are quite convenient. I see the suggestion that some of these are offered by Lodash or other repos, but I'm not immediately seeing (or at least not understanding) the direct equivalents, so it would be useful to have these more spelled out in the deprecation guide.

I also use these functions extensively during interactive debugging, where the fact they are array prototype extensions is a huge boost, as it is near impossible to import an arbitrary function from a third-party library in the browser's JS console. I suppose I could resort to native Array methods, but then I have to write out a lambda function every time, which is tedious and I'm sure will leave me cursing the loss of these beloved convenience functions. Are there any thoughts on how best to cope with this loss of productivity? EDIT: I'm genuinely looking for suggestions here -- if my app imports a Lodash function, am I able to access that same function directly from JavaScript console as easily? Do I just need to export my own custom object containing the functions to the global scope? If that's the answer, then so be it -- I mainly wanted to recognize the convenience value here and solicit input on the best way to retain that convenience after this change.

[jrjohnson]

Forgot a big gotcha in removing these as extensions and using utilities, optional chaining doesn't do the job anymore. If you currently have return this.values?.mayBy('id') and this.values isn't set you'll get undefined. But doing return mapBy(this.values ?? [], 'id') will return []. When combined with @robbytx's comment I wonder if there are some of these worth keeping around. The *by ones are very useful.

[chriskrycho]

Thanks for your comments! We'll want to make sure those details get captured as we post about these deprecations.

That said, I think the RFC addresses the trade offs clearly, and as a result there is zero interest in keeping around any of them as prototype extensions... They are, presently anyway, available separately as utilities importable from the @ember/array, and if you want to muck with the prototype yourself, you are welcome to do that! But it is not a thing we want to do in core anymore, and what's more (as discussed a bit upthread) getting rid of these here is a necessary first step to getting rid of EmberArray in favor of just using native arrays or TrackedArray!

We will make sure to talk through these points once more at Framework Core on Friday, though! This is what FCP is for!

[runspired]

Lastly, as @runspired mentioned Ember Data 4.8 is going to handle it's array-like stuff better, in fact the deprecations of methods in ED (#745) are what caused me to get into this in our app and just replace them everywhere. However, until #846 is fully realized (and maybe some other ED RFCs) you're going to get a mix of types coming from resolving models and their relationships and passing those values around.

The implementation of #846 is fully realized and in #745 and the ember-data 4.7 release. If there's a bug let's fix it :)

Updating Ember Data @hasmany relationships still requires the pushObject(s), this may be fixed in a more recent version, I'm not sure. But it means a global search/replace isn't an option.

This should not at all be required in 4.7

I found myself pretty constantly messing with adding .slice() and removing it from various thing because I was wrong about the source of that data and would get errors from return [...posts, newPost] type code.

Would love an example / to hear more about this. I'm not sure what you're pointing out. Pre EmberData 4.7 using some native array APIs could lead you to getting the backing content (InternalModel/Identifier depending on version) but starting in 4.7 regardless of native array API used you should get the record instance.

[runspired]

They are, presently anyway, available separately as utilities importable from the @ember/array, and if you want to muck with the prototype yourself, you are welcome to do that! But it is not a thing we want to do in core anymore

To echo what @chriskrycho is saying here, mutating the Array Prototype has historically (and I believe still?) caused arrays to opt into a de-opted slow state. There's a large number of such pitfalls around array APIs, even to the extent that the single use once of an array API on an instance of an array in a specific way can cause all arrays to not only de-opt but for that de-opt to persist across browser sessions. Ending our mutation of array prototypes and array instances entirely is very important.

[smilland]

Thank you @jrjohnson for sharing the deprecation experiences and gotchas. Contributions to the deprecation guide are heartily welcomed and appreciated as well ❤️ . ember-learn/deprecation-app#1192

[smilland]

Hi @ef4 @chriskrycho , this RFC has been in FCP for over a week now. Let me know if there is anything else pending and what's the next step. Thanks!

[chriskrycho]

Nope, all good on your end! We just haven't had a chance to discuss it again: a lot of folks were out last week for EmberFest. It's at the top of the agenda for this week's meeting!

[chriskrycho]

We discussed this again at Framework Core today and are merging it (…once we get the lints fixed)! Thanks again, @smilland!

[smilland]

@chriskrycho That's awesome! Fixed lint. Thank you for all the guidance, support, and discussions. ❤️ ❤️

[chriskrycho]

Merged. Thanks again! Excited to see it implemented.

[bertdeblock]

Sweet! Already opened ember-cli/ember-cli#10017 for Ember CLI.

[johanrd]

@smilland hi, the "rendered text" link in the original post leads to a 404. Is there a copy anywhere else? thanks,

Edit: found a copy here: https://github.com/smilland/rfcs/blob/734fa12241674cca56ea7a3b30b6eb027ebde752/text/0848-deprecate-array-prototype-extensions.md

[ef4]

This will need to grapple with some of the issues in #864 as well.

When array prototype extensions are disabled, Ember.A gains the behavior of mutating its argument to add those tensions back in. This can have weird and wonderful effects at a distance that are hard to deal with.