Conversation
|
@hzoo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loganfsmyth, @Alxpy and @existentialism to be potential reviewers. |
| return { | ||
| inherits: syntaxClassProperties, | ||
|
|
||
| capabilities: ["classProperties"], |
There was a problem hiding this comment.
Opt-in so 3rd party plugins can also provide capabilities/dependencies. Right now it's just a string but maybe it should be a versioned number?
|
I like it, thanks for working on this! As I reviewed the |
|
Ok as discussed on slack, maybe
|
node_modules are required to exists so that flow can inference the types
Codecov Report
@@ Coverage Diff @@
## 7.0 #5735 +/- ##
==========================================
+ Coverage 85.09% 85.14% +0.04%
==========================================
Files 284 284
Lines 9911 9942 +31
Branches 2768 2769 +1
==========================================
+ Hits 8434 8465 +31
+ Misses 976 974 -2
- Partials 501 503 +2
Continue to review full report at Codecov.
|
| plugins.forEach((pluginTuple, index) => { | ||
| const plugin = pluginTuple[0]; | ||
| if (!plugin.key) { plugin.key = `${unnamedPluginPrefix}${index}`; } | ||
| }); |
There was a problem hiding this comment.
we probably don't need this bit anymore, given that we're making name compulsory. i'll remove it in my naming PR
| if (plugin.capabilities) { | ||
| plugin.capabilities.forEach((capability) => { | ||
| const existingPluginName = capabilitiesToPluginIdMap[capability]; | ||
| if (existingPluginName !== undefined) { |
There was a problem hiding this comment.
prob safer to just say if (existingPluginName), this way empty strings can also be caught. (although that shouldn't be allowed in the first place).
| throw new Error("Plugin .after must be an array"); | ||
| } | ||
| if (!Array.isArray(plugin.before)) { | ||
| throw new Error("Plugin .before must be an array"); |
There was a problem hiding this comment.
Naive thinking perhaps, but judging by the conditionals above this one, it may freak out if plugin.before / plugin.after isn't present.
| this.capabilities = plugin.capabilities; | ||
| this.dependencies = plugin.dependencies; | ||
| this.after = plugin.after; | ||
| this.before = plugin.before; |
There was a problem hiding this comment.
based on my prev comment, might wanna do a this.x = plugin.x || []; to account for cases where its not present?
|
hi @hzoo, on #5911, I've synced work on this branch with Once that's done, I can get #5842 ready too - it's essentially good to go, but is failing because it depends on this branch which is currently failing. |
hulkish
left a comment
There was a problem hiding this comment.
I feel like maybe a stronger approach here could be to introduce some kind of lifecycle hooks concept... to start with would be those known plugins which require one in particular to come before the other.
|
hello, trying to catch up on this. What's blocking this today? is the TODO at the top up to date? |
Fixes #5623
Summary
Currently this is just the minimal change necessary to make class properties and decorators work (not a final approach or anything and not done yet)
This PR adds 2 more properties to the Plugin object:
capabilities: an array of values (currently a string, maybe better as a number/semver like in package.json) to represent what a plugin can transformdependencies: an array of values to represent which syntax needs to be transformed after this plugin is run (it depends on that syntax being there beforehand so it should go first)before/after: an array of values to represent which plugin capabilities should be run before/afterTODO
maybe expose some symbols instead of a string? likebabel.capabilities.arrowFunctions = Symbol()handle cycles (dep A -> dep B -> dep C -> dep A) - just error with a good error messagethis probably isn't an issue and/or we will fix it when it's reportedUse case
cc @benjamn, @rwjblue