-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Require proposal flag for pipeline plugin #8196
Require proposal flag for pipeline plugin #8196
Conversation
I think this is a reasonable approach. |
api.assertVersion(7); | ||
|
||
if (typeof proposal !== "string" || !proposals.includes(proposal)) { | ||
throw new Error("'proposal' must be one of: " + proposals.join(", ")); |
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.
Yep looks good, wonder if we should introduce a url to show what the options are and why etc.
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.
I also plan on updating the README. Is there any other place where we should include that documentation?
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.
I think the readme is good. We should introduce a new general doc on the page explaining how we want to think about proposals moving forward and how they are implemented though. Kinda like babel/proposals#26 but I didn't get to finishing it, not sure.
@loganfsmyth @hzoo This also breaks the stage-0 preset, which would need this configuration. Is that acceptable? If so, I can update the tests to take the config. |
Yeah we are going to remove the stage presets because of this issue #7770. Same issues we had with decorators actually. Not really worth the trouble anymore so people should opt-into what they want or make their own preset. |
Ok cool, I'll make those changes this evening. |
Even if we are going to deprecate the stage-0 preset, I'd prefer to add a flag which is passed down to this plugin, similarly to what I did with the |
@nicolo-ribaudo Yeah, that the test that's failing, so I'll make sure that works when I fix it. |
This is going to be required so we can add support for other proposals, as well as later set the accepted proposal as the default. Update stage-0 / -1 plugins with `pipelineProposal` to thread down to the plugin.
584086b
to
940d9b2
Compare
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8431/ |
1 similar comment
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8431/ |
Documentation update: babel/website#1700 |
I think all the open question have been addressed, so this should be ready to go. |
Bumping this PR. Would like to get this landed so we can refresh our branch with the incoming pipeline changes. |
@@ -1,7 +1,7 @@ | |||
{ | |||
"plugins": ["external-helpers", ["proposal-class-properties", {"loose": true}]], | |||
"presets": [ | |||
["stage-0", { "decoratorsLegacy": true }], | |||
["stage-0", { "decoratorsLegacy": true, "pipelineProposal": "minimal" }], |
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.
Unrelated to the PR but I wonder why the stage-0 preset is even necessary to use in this test?
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.
I have no idea, but it did catch that I broke state-0 so maybe that's ok?
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, it is ok. I often use stage-0
in tests for lazyness 😛
(please update the docs at https://github.com/babel/website/tree/master/docs) |
@nicolo-ribaudo PR is already here: babel/website#1700 |
I'm opening this now to get feedback early. I'm not sure if the logic should live in the proposal plugin, the syntax plugin, or the parser? (or all three?)
Is
proposal
an acceptable flag name?Also, this breaks the current stage-0 plugin as well, which I don't know if you're going to keep. Is that going to be an issue?