Skip to content
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

Converge extension priority parity with that of webpack #1080

Merged
merged 2 commits into from Sep 11, 2018

Conversation

eliperelman
Copy link
Member

@eliperelman eliperelman commented Sep 6, 2018

Fixes #1067.

This tries to at least get us close to the correct extension priority with what users should expect. It sucks that this list just keeps growing, but at the moment I think doing something different would be much harder than just hardcoding this right now.

@eliperelman eliperelman added this to the v9 milestone Sep 6, 2018
@eliperelman eliperelman self-assigned this Sep 6, 2018
@eliperelman eliperelman requested a review from edmorley Sep 6, 2018
Copy link
Member

@edmorley edmorley left a comment

Travis is failing at the moment.

Also, looking locally I think there is another existing bug, in that the order in source is not preserved, since it's stored in neutrino.options.extensions as a Set():

let moduleExtensions = new Set(source);

@@ -4,7 +4,7 @@ module.exports = {
// Modifying a value here should have an accompanying change there as well.
// We can't pull in neutrino there as that would potentially give us
// conflicting versions in node_modules.
source: ['js', 'jsx', 'vue', 'ts', 'tsx', 'mjs'],
source: ['wasm', 'mjs', 'jsx', 'vue', 'tsx', 'ts', 'web.jsx', 'web.js', 'js'],
Copy link
Member

@edmorley edmorley Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should put the web.js{x} entries here, since they are react-specific (similar to how we don't put the react-native resolve.alias in any other presets.

Copy link
Member

@edmorley edmorley Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah the rest of my comment got lost, since I forgot to press update comment before submitting the review. Trying again:

The value for source ends up in neutrino.options.extensions, which is used for neutrino.config.resolve.extensions as expected, but also used by the Mocha preset for the Babel extensions regex, and the Jest preset for moduleFileExtensions. Do the latter two uses support wasm, or does wasm need to just be for neutrino.config.resolve.extensions (ie webpack) only?

(Similar to how json is added for webpack only:

.resolve
.extensions
.merge(neutrino.options.extensions.concat('json').map(ext => `.${ext}`))
.end()
.end()
)

Copy link
Member Author

@eliperelman eliperelman Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is yes, so that those files can be imported from tests. Correct me if you think this assumption is invalid.

@@ -93,7 +93,7 @@ module.exports = class Project extends Generator {
// here should have an accompanying change there as well. We can't pull
// in neutrino here as that would potentially give us conflicting versions
// in node_modules.
const lint = 'eslint --cache --ext js,jsx,vue,ts,tsx,mjs src';
const lint = 'eslint --cache --ext wasm,mjs,jsx,vue,tsx,ts,web.jsx,web.js,js src';
Copy link
Member

@edmorley edmorley Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the .js{x} entries are a superset of .web.js{x}, making the latter redundant for this case specifically?

Also, can ESLint parse .wasm? I thought the files were binary.

Copy link
Member Author

@eliperelman eliperelman Sep 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the .js{x} entries are a superset of .web.js{x}, making the latter redundant for this case specifically?

Good point. This is the primary reason why I had added these entries to the neutrino source extensions array.

Also, can ESLint parse .wasm? I thought the files were binary.

Oh, my bad. I thought these were JS-superset files.

I'll make these revisions.

@eliperelman eliperelman requested a review from edmorley Sep 6, 2018
@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented Sep 6, 2018

The problem wasn't that source order is not preserved, but rather the tests are expecting the results to be in a different order than defined. I updated the patch to fix the tests expected order.

@@ -4,7 +4,7 @@ module.exports = {
// Modifying a value here should have an accompanying change there as well.
// We can't pull in neutrino there as that would potentially give us
// conflicting versions in node_modules.
source: ['js', 'jsx', 'vue', 'ts', 'tsx', 'mjs'],
source: ['wasm', 'mjs', 'vue', 'jsx', 'tsx', 'ts', 'js'],
Copy link
Member

@edmorley edmorley Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we drop .ts and .tsx for now (here and for the eslint command), given no core Neutrino preset supports TypeScript, and the inclusion of those extensions apparently made things worse in #1044 (or at least caused confusion)?

Copy link
Member Author

@eliperelman eliperelman Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this in a follow-up.

@eliperelman eliperelman merged commit 4c95c21 into neutrinojs:master Sep 11, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants