Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate array prototype extensions #20702

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

simonihmig
Copy link
Contributor

@simonihmig simonihmig commented Jun 3, 2024

As per RFC848.

This was a bit trickier than I actually expected, here is what I did and failed to do:

  • The RFC says the deprecation should trigger when EmberENV.EXTEND_PROTOTYPES is not false. But given that there is some data munging happening here that is turning EXTEND_PROTOTYPES: false to become EXTEND_PROTOTYPES: { Array: false }, which I was not eager to change, the deprecation is skipped for both of these. I think that is ok?
  • I tried to add the deprecation to the env.ts file where the EXTEND_PROTOTYPES config is evaluated (1st commit)
  • this didn't work, as it was causing a circular module dependency between that module and deprecations/index.ts, so I moved the deprecation call to the latter. Not ideal, but I had no better idea. (2nd commit)
  • I have no test that is asserting for the deprecation to be issued, because with that implementation the deprecation is triggered when the module is loaded, so way before any test could call expectDeprecation. Any idea how to add a test here?
  • Given that I was not able to silence the deprecation by a expectDeprecation, I added RAISE_ON_DEPRECATION=false to the test matrix scenarios that have EXTEND_PROTOTYPES to make them be able to actually run. (3rd commit)

@simonihmig simonihmig force-pushed the array-prototype-deprecation branch 3 times, most recently from fafcfba to 7981199 Compare June 5, 2024 19:12
@simonihmig simonihmig force-pushed the array-prototype-deprecation branch from 7981199 to d06a63b Compare June 5, 2024 19:14
@simonihmig simonihmig force-pushed the array-prototype-deprecation branch from d06a63b to 09dc249 Compare June 5, 2024 19:20
@simonihmig simonihmig marked this pull request as ready for review June 5, 2024 19:35
@@ -129,6 +129,16 @@ export const DEPRECATIONS = {
enabled: '5.10.0',
},
}),
DEPRECATE_ARRAY_PROTOTYPE_EXTENSIONS: deprecation({
id: 'deprecate-array-prototype-extensions',
url: 'https://deprecations.emberjs.com/id/deprecate-deprecate-array-prototype-extensions',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This points to ember-learn/deprecation-app#1192, which we also need to get merged.

@kategengler kategengler requested a review from ef4 June 10, 2024 19:14
@ef4 ef4 merged commit 756f0e3 into emberjs:main Jun 14, 2024
26 checks passed
@ef4
Copy link
Contributor

ef4 commented Jun 14, 2024

Thanks @simonihmig.

We should backport this beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants