-
Notifications
You must be signed in to change notification settings - Fork 386
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(versioning): remove object rest spread in v59 (BREAKING CHANGE) #3592
feat(versioning): remove object rest spread in v59 (BREAKING CHANGE) #3592
Conversation
This change also technically relies on #2948 because we need to drop IE11 support. |
Un-merged. |
package.json
Outdated
"@babel/core": "^7.22.5", | ||
"@babel/helper-explode-assignable-expression": "^7.18.6", | ||
"@babel/preset-env": "^7.22.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these unused? Wasn't sure how it is related to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I made things confusing by merging this PR with #2948. I've removed those unrelated changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the object spread related changes LGTM
version: '11', | ||
label: 'sl_edge_compat', | ||
browserName: 'MicrosoftEdge', | ||
version: '18', | ||
compat: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is compat
field needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2948. Yes, because I'm still using "compat" to test legacy browsers in Karma. (I.e. changing the meaning of "compat" for those tests.)
263d86a
to
87abd30
Compare
OK, apparently Edge 18 does not support object rest spread. |
expect(code).not.toContain('...a'); | ||
} else { | ||
expect(code).toContain('...a'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test looks a little clunky. Will additional versions be added to the [58, 59]
array above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, testing 58 vs 59 will be sufficient forever.
|
||
if (process.env.API_VERSION <= 58) { | ||
// babel polyfill format | ||
expect(test.toString()).not.toContain('...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a clever way to test this, without requiring access to the raw compiled asset itself.
I don't think we can do this in 246 due to the Edge issue. |
Details
Fixes #3577
Removes the object rest spread Babel transformation when the API version is >=58.
Does this pull request introduce a breaking change?
In a sense, yes, it is breaking, because it is observable. See #3577
Does this pull request introduce an observable change?
GUS work item
W-13653003