Replace deprecated Rollup option 'legacy' with custom plugin. #13356
Conversation
The `legacy` option was added to the Rollup build system (for www builds) in facebook#11469 because the internal transforms break on getters. As of version 0.60.0 of Rollup, `legacy` support is removed ([source](rollup/rollup#2141), [changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#0600)). This PR adds a custom Rollup plugin to replicate the `legacy` behaviour. Note that the `legacy` option preformed several transformations to support IE8. The custom Rollup plugin only replicates the replacement of getters ([original code here](https://github.com/rollup/rollup/blob/349677ceee9d4bbccb5b2f72e653270cb2b0ce51/src/ast/variables/NamespaceVariable.ts#L63)). Also note that getters are only generated in a very specific case by Rollup. That is, when doing namespace imports (`import * as name from 'origin'`) and one of the imported variables is reassigned at some point (an example of this can be found in the [old Rollup tests](https://github.com/rollup/rollup/tree/349677ceee9d4bbccb5b2f72e653270cb2b0ce51/test/form/samples/legacy-getter)). At this time, the only getter generated by React code is `ReactDOMEventListener._enabled`.
|
Seems brittle. |
|
Might be relevant. I haven't checked but I also don't have reasons to believe it got fixed. |
|
@gaearon Currently Babel runs before Rollup, but I'll try to add a new Babel pass after the Rollup bundle to remove the getters. |
|
Yeah. Just for FB bundle, and just with that single transform. |
The Babel plugin is added in a new Babel pass after the Rollup bundle. Note that running Babel after Rollup adds an indentation level because of the `if (__DEV__)` wrapper.
|
Is there a reason we're not using some stock Babel transform for compiling getters according to the spec? |
scripts/rollup/build.js
Outdated
| ], | ||
| }; | ||
| const result = babelCore.transform(bundleCode, babelOptions); | ||
| fs.writeFileSync(mainOutputPath, result.code, 'utf8'); |
There was a problem hiding this comment.
I think running babel inside transformBundle without extra read/write would be a better idea.
|
@elas7 If you prefer to use plugin over custom transformation there is an unofficial one: Source code if needed: |
|
@Simek Sadly I can't use that unofficial plugin because it only transforms Thanks, though. |
This is more efficient because we don't have to do an extra read/write.
scripts/babel/remove-getters.js
Outdated
| const keyNode = path.node.key; | ||
| const isValidKey = t.isIdentifier(keyNode); | ||
| if (!isValidKey) { | ||
| return; |
There was a problem hiding this comment.
Can we throw here instead?
scripts/babel/remove-getters.js
Outdated
| t.isReturnStatement(bodyNode.body[0]) && | ||
| t.isIdentifier(bodyNode.body[0].argument); | ||
| if (!isValidBody) { | ||
| return; |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
The
legacyoption was added to the Rollup build system (for www builds) in #11469 because the internal transforms break on getters. As of version 0.60.0 of Rollup,legacysupport is removed (source, changelog). This PR adds a custom Rollup plugin to replicate thelegacybehaviour.Note that the
legacyoption performed several transformations to support IE8. The custom Rollup plugin only replicates the replacement of getters (original code here).Also note that getters are only generated in a very specific case by Rollup. That is, when doing namespace imports (
import * as name from 'origin') and one of the imported variables is reassigned at some point (an example of this can be found in the old Rollup tests). At this time, the only getter generated by React code isReactDOMEventListener._enabled.