Make inline-require transform compatible with other transforms

facebook / fbjs

A collection of utility libraries used by other Meta JS projects.

MIT License

1.95k stars 313 forks source link

Make inline-require transform compatible with other transforms #237

Closed bvaughn closed 7 years ago

bvaughn commented 7 years ago

My recent changes to the inline-requires transform caused unintended problems when ran alongside other transforms (eg 'babel-plugin-transform-es2015-modules-commonjs').

// This...
import Imported from 'XHRHttpError';

// Would be transformed into this...
var _XHRHttpError2 = babelHelpers.interopRequireDefault(_XHRHttpError);

The fix for this is to avoid re-processing the entire AST as we may interfere with modifications made by other plug-ins. Instead the 'inline-requires' transform tracks Identifiers and updates them (only) if we later remove a require they were depending on.

bvaughn commented 7 years ago

cc @spicyj

sophiebits commented 7 years ago

Test pls.

bvaughn commented 7 years ago

Not sure how to repro the problem in a test within the context of this project. 🙁 Will have to look into it.

zpao commented 7 years ago

We should maybe be transforming imports here as well… cc @josephsavona

bvaughn commented 7 years ago

I've added the 'es2015-modules-commonjs' transform to the 'inline-requires' test to catch the previous failure and verify the fix.

bvaughn commented 7 years ago

@zpao If we decide to do import as well, maybe we could follow up on that with a separate PR (so as to unblock things internally)? I'd be happy to help with this.

bvaughn commented 7 years ago

Ok @spicyj, back to you!

bvaughn commented 7 years ago

Many thanks.

Let me know once babel-preset-fbjs@2.1.2 is out and I'll update my sync and try again. 😁