Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

[v9] options.extensions not used #1404

Closed
rkostrzewski opened this issue May 29, 2019 · 6 comments · Fixed by #1477
Closed

[v9] options.extensions not used #1404

rkostrzewski opened this issue May 29, 2019 · 6 comments · Fixed by #1477
Labels
Milestone

Comments

@rkostrzewski
Copy link

rkostrzewski commented May 29, 2019

Bug

Please try to answer the following questions:

  • What version of Neutrino are you using?
    9.0.0-rc.3
  • Are you trying to use any presets? If so, which ones, and what versions?
    @neutrinojs/react@9.0.0-rc.3 (should be visible with compile-loader only)
  • Are you using the Yarn client or the npm client? What version?
    npm@6.7.0
  • What version of Node.js are you using?
    node@10.8.0
  • What operating system are you using?
    macOS
  • What did you do?
  1. Run npx @neutrinojs/create-project@9.0.0-rc.3 with only React on
  2. Edited .neutrinorc.js to look like this:
const react = require('@neutrinojs/react')

module.exports = {
  options: {
    root: __dirname,
    extensions: ['mjs', 'jsx', 'js', 'ts'], // <- added 'ts' extension
  },
  use: [
    react(),
  ],
}
  1. Created empty src/index.ts file and removed src/index.js/
  2. Run webpack --mode production
  • What did you expect to happen?
    Build finishes successfully because src/index.ts is resolved by webpack and processed by babel
  • What actually happened, contrary to your expectations?
    Build fails with error:
ERROR in multi ./src/index
Module not found: Error: Can't resolve '/Users/rkostrzewski/GitHub/coinsy/web/src/index' in '/Users/rkostrzewski/GitHub/coinsy/web'
 @ multi ./src/index index[0]

Based on quick investigation this might occur because options.extensions is not used in Neutrino.getOptions instead moduleExtensions variable is used that is initialised only with source from neutrino/extensions module (changing that declaration to include options.extensions yields a successful build however on v8 the code looks the same yet extension 'foo' works in v8)

@edmorley edmorley added this to the Neutrino 9 milestone May 31, 2019
@edmorley edmorley added the bug label May 31, 2019
@edmorley
Copy link
Member

Hi! Thank you for reporting. Yeah this looks like a bug.

@eliperelman it seems like we should be setting the default extensions list here (rather than declaring moduleExtensions the line before):

const options = {
debug: false,
...clone(opts),
};

...and then referring to options.extensions (instead of moduleExtensions) here:

Object.defineProperty(options, 'extensions', {
enumerable: true,
get() {
return [...moduleExtensions];
},
set(extensions) {
moduleExtensions = new Set(extensions.map(ext => ext.replace('.', '')));
},
});

We should also add some tests for this, since there currently aren't any for extensions here:
https://github.com/neutrinojs/neutrino/blob/master/packages/neutrino/test/api_test.js

Thoughts?

@eliperelman
Copy link
Member

@edmorley it's funny you mention that, because I started doing exactly that yesterday. Then I remembered that the order of these extensions is very important, so I'm wondering if setting this in options should overwrite instead of merge?

@edmorley
Copy link
Member

Ah yes I think it should override.

@edmorley
Copy link
Member

edmorley commented Jun 9, 2019

@eliperelman hi :-) Did you have a WIP for this locally?

@timkelty
Copy link
Contributor

timkelty commented Oct 7, 2019

@eliperelman, @edmorley If no one has a WIP of this, I can take a stab this week!

@timkelty
Copy link
Contributor

Thanks for the clear bug report, @rkostrzewski. Just pushed a PR: #1477

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

4 participants