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
Add optional-chaining and nullish-coalescing to preset-env #10811
Add optional-chaining and nullish-coalescing to preset-env #10811
Conversation
Druotic
commented
Dec 4, 2019
Q | A |
---|---|
Fixed Issues? | Fixes #10809 |
Patch: Bug Fix? | N |
Major: Breaking Change? | N |
Minor: New Feature? | Y |
Tests Added + Pass? | Y |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
@@ -299,6 +299,10 @@ | |||
"opera": "53", | |||
"electron": "3.1" | |||
}, | |||
"proposal-optional-chaining": { |
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.
We should also add a plugin syntax map to the pluginSyntaxMap
const pluginSyntaxMap = new Map([ |
So that if babel is configured with
{
"presets": [
["@babel/env", { "targets": { "browserslist": "chrome 80" } }]
]
}
Babel would not transform them at all since it gets native support.
By doing so it means we need to add @babel/syntax-proposal-optional-chaining
as dependencies, too.
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.
Added - hopefully I did that correctly. Being new to this project - I must admit, I don't fully understand all of the machinery here, so assume the worst 😂
Thanks for the quick feedback, as well 👍
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.
Hmm, am I correct in assuming I should also add the syntax plugin to the notIncludedPlugins
in babel-present-env-standalone
?
const notIncludedPlugins = { |
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.
You are doing great! And we should also add the syntax plugin to preset-env/src/available-plugins.
Alright - should be good for a re-review. I combined the other PR (nullish coalescing) into this one and assumed the same pattern was necessary. |
"transform-named-capturing-groups-regex": "RegExp named capture groups", | ||
"transform-member-expression-literals": "Object/array literal extensions / Reserved words as property names", | ||
"transform-property-literals": "Object/array literal extensions / Reserved words as property names", | ||
"transform-reserved-words": "Miscellaneous / Unreserved words", | ||
"proposal-nullish-coalescing-operator": "nullish coalescing operator (??)", |
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.
Can you keep this alphabetically ordered?
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.
Not a blocker for this PR, but would be nice if we can enforce this with ESLint, even if it's just enabling the rule using comments for this object.
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.
Yeah that would be cool! But let's do it in a separate 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.
Oops - I didn't see this reply until after making the changes. I selectively added the sort-keys
rule to a few files which resulted in some noise/sorting of existing keys. It's in a separate commit for easy review. If it's too much, I can pull it out into another 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.
It's still okay to review since the changes are related to a few files.
["proposal-unicode-property-regex", null], | ||
["proposal-json-strings", "syntax-json-strings"], | ||
["proposal-nullish-coalescing-operator", "syntax-nullish-coalescing-operator"], |
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.
Also here
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.
Thanks!
@Druotic FYI, we will merge this PR when we are ready for the next minor release (it could take a while) |
I think a lot of people are eagerly awaiting this, any chance of doing this soonish? :) |
@@ -3,12 +3,15 @@ | |||
|
|||
const proposalPlugins = {}; | |||
|
|||
// Please keep these in alphabetical order! |
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.
If you want to enforce these tuples being in alphabetical order, you could use /* eslint sort-keys: "error" */
like you do in available-plugins.js
, and create an object representing these plugins where the order will be enforced.
Then you could transform it for the new Map()
:
const pluginSyntaxObject = {
"proposal-async-generator-functions": "syntax-async-generators",
"proposal-json-strings": "syntax-json-strings",
"proposal-nullish-coalescing-operator": "syntax-nullish-coalescing-operator",
"proposal-object-rest-spread": "syntax-object-rest-spread",
"proposal-optional-catch-binding": "syntax-optional-catch-binding",
"proposal-optional-chaining": "syntax-optional-chaining",
"proposal-unicode-property-regex": null,
}
const pluginSyntaxMap = new Map(Object.entries(pluginSyntaxMap))
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.
Good call - updated. I also resolved new conflicts with master.
Note that if you really want to test on the new language features, you can always enable these features before Install via
Add these plugins to your babel config {
"presets": ["@babel/preset-env"],
"plugins": [
"@babel/plugin-proposal-nullish-coalescing-operator",
"@babel/plugin-proposal-optional-chaining"
]
} |
|
||
module.exports = { proposalPlugins, pluginSyntaxMap }; | ||
const pluginSyntaxMap = new Map(Object.entries(pluginSyntaxObject)); |
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.
Unfortunately Object.entries
is not supported in Node 6 (behind a flag), could you add a todo item and restore it to Map
constructor? We can apply this change on Babel 8.
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.
Or maybe someone could write a feature request in the eslint repo to make sort-keys
work with the Map
and Set
constructors
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 for that confusion! I wasn't aware of the Object.entries
limitation.
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.
@msholty-fd I didn't realize it before I read the CI error. The discussion above is still constructive that we have new eslint rules idea.
babel/babel#10811 was merged and released in babel 7.8.0