Support wider set of web extensions - #2211
Conversation
33b6adc to
b14fa2b
Compare
lukastaegert
left a comment
There was a problem hiding this comment.
I think this is a good idea but why not actually make this a configuration? For once, the more file types we add, the more things like extension priority starts to matter. And at the same time, users could tailor it to their needs.
I would also suggest to give .mjs the highest priority by default, not .js.
|
This isn't affecting what extensions can be loaded in the resolver at all (that remains Ideally I would actually want to pull in the MIME module here and accept anything that is registered as a MIME, but due to the bundle requirements for Rollup, would rather let this grow into an internal registry tailored to the JS ecosystem. Perhaps I can try and make the first pass more comprehensive if that would help? |
| '.js': true, | ||
| '.mjs': true, | ||
| '.jsx': true, | ||
| '.ts': true |
There was a problem hiding this comment.
typescript forces to use tsx extension with jsx
Ah you are right. I had those two connected in my head but yes, those are separate things. I think this is ok but @TrySound's suggestion is surely valid. |
|
I've actually modified the approach to no longer need an extension lookup, and instead flexibly determine the extension - basically by stripping the resolved extension from the alias. |
7228a32 to
c8fd10f
Compare
lukastaegert
left a comment
There was a problem hiding this comment.
I like the update but still think we should add a configuration, see my comment
| } | ||
| found = findFile(file + '.js', preserveSymlinks); | ||
| if (found) return found; | ||
| return findFile(file + '.mjs', preserveSymlinks); |
There was a problem hiding this comment.
But going back to my original comment, wouldn't this allow .mjs to be used without an extension while giving precedence to .js in these situations which is probably not in rollup's best interest? If we do this, I think these two extensions + their precedence should be configurable as an array with the default being ['mjs', 'js']. On the other hand, this would FINALLY settle some long-standing feature requests, cf #1894
There was a problem hiding this comment.
I forgot we were supporting mjs here actually, also only just saw this again when refactoring the above. I've fixed this up to at least check in the right order, meaning yes we can properly support .mjs now!
Personally, I don't think we should have an array here - anything that configures the resolver should also configure the loader to support the translate for that file extension. .mjs and .js are the only valid registered MIMES for JavaScript files, with a well-defined precedence, and anything else will be a different module format.
There was a problem hiding this comment.
Makes sense 👍 I still think that we may want to configure this at some point (if people ask about it?) but with this behaviour in place, this can definitely wait for another PR. I just remember struggling with some linter settings around this behaviour at work and being happy that Webpack has an option to configure just that.
8098238 to
69f94e4
Compare
From #2206, we can strip a wider list of well-known web extensions.
I'd be happy to expand this list as well to anything that could be considered as a MIME.