feat(node-resolve): Increase the support for .mjs/.cjs #1454
Conversation
|
Test has already been added. |
|
@fierflame please fix the prettier issue prettier -w packages/node-resolve/ |
Import .mts or .cts files in TypeScript files requires passing through .mjs or .cjs flies.
This commend has been run. |
Btw. it is |
| ['.mjs', '.mts'], | ||
| ['.cjs', '.cts'] |
There was a problem hiding this comment.
I'm going to let other reviewers make the final call on this; it doesn't feel right that node-resolve should be looking for or accepting typescript files. That feels like the domain of the typescript plugin exclusively.
There was a problem hiding this comment.
I am also not 100% convinced here. After all, @rollup/plugin-typescript will happily resolve .js extensions to .ts files. As a matter of fact, at least in the Rollup sources it does not seem to have problems if I rename files to use .mts extensions and import them as .mjs. So what problem are we solving here?
There was a problem hiding this comment.
Based on the outcome of this we may want to revisit the lines above this which appear to be solving a similar problem
There was a problem hiding this comment.
Oh, I overlooked this. Then maybe it was node-resolve after all that allowed .ts files to be imported as .js. In that case, I guess it is only consistent to also handle the new extensions. But we should consider merging the two blocks.
| @@ -0,0 +1,11 @@ | |||
| // To make this very clearly TypeScript and not just CJS with a CTS extension | |||
There was a problem hiding this comment.
same comment applies here
|
Hi, I'm looking forward to this feature. Is there anything left to validate and merge this? |
|
@onigoetz yes. all of the unresolved comments. please take a closer look at a PR before asking that question in the repo in the future. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@lukastaegert what's your take on merging this as-is or requiring @fierflame to make some adjustments? |
There was a problem hiding this comment.
While the change looks good to me in general, I would still like for the new code to be merged with the block directly above. Also, that code checks if an importer is present, which also makes sense for the new code. If no importer is present, then this is an entry point and we do not want to make these checks.
|
This one should be completely covered by #1498. If that is not the case, feel free to reopen. |
Rollup Plugin Name:
node-resolveThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
Import .mts or .cts files in TypeScript files requires passing through .mjs or .cjs flies.