fix: fully specify resolve path with .js extension

[borisdiakur]

Relatively imported modules need to have the .js or /index.js extension to be added to the import path in order to resolve an actual path, when working with esm in environments, where a fully specified path is the default (such as in a create react app with webpack 5, which, if not ejected and changed, has resolve.fullySpecified set to true).

Fixes: #319

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features) -> adjusted existing tests to include .js extensions on relevant import statements
• Docs have been reviewed and added / updated if needed (for bug fixes / features) -> reviewed; no additions
• Build (npm run build) was run locally for affected output targets
• Tests (npm test) were run locally and passed
• Prettier (npm run prettier) was run locally and passed -> reverted changes made by prettier as they were unrelated to my changes (code style should be fixed in separate PR)

Pull request type

Please check the type of change your PR introduces:

• Bugfix

What is the current behavior?

In Nuxt 3 I get the following error when using the vue output target:

[nuxt] [request error] [unhandled] [500] Cannot find module '/Users/boris/workspace/work/liquid/liquid-sandbox-nuxt-tailwind/.output/server/node_modules/@emdgroup-liquid/liquid/dist/vue-component-lib/utils' imported from /Users/boris/workspace/work/liquid/liquid-sandbox-nuxt-tailwind/.output/server/node_modules/@emdgroup-liquid/liquid/dist/vue.js

In a create-react-app I get the following error when using the react output target:

ERROR in ./node_modules/@emdgroup-liquid/liquid/dist/react.js 6:0-61
Module not found: Error: Can't resolve './react-component-lib' in 'C:\Users-\WebstormProjects-\node_modules@emdgroup-liquid\liquid\dist'
Did you mean 'index.js'?
BREAKING CHANGE: The request './react-component-lib' failed to resolve only because it was resolved as fully specified
(probably because the origin is strict EcmaScript Module, e. g. a module with javascript mimetype, a '.mjs' file, or a '.js' file where the package.json contains '"type": "module"').
The extension in the request is mandatory for it to be fully specified.
Add the extension to the request.

Issue URL: #319

What is the new behavior?

• added .js or /index.js extension to relative import and export paths
• added .js or /index.js in tests
• added moduleNameMapper setting to vue output target test config (react output target still works without changes)

Does this introduce a breaking change?

• No

Other information

See also microsoft/TypeScript#16577

[sean-perkins]

Hello @borisdiakur thanks for this contribution!

I'm not sure I understand why these changes are necessary. Ionic Framework uses the output targets to ship to both React and Vue, and our React starters take advantage of create-react-app as well. There has been no reported issues and our starters are functioning as expected.

The source files you changed are TypeScript files that are copied into the destination project. They are not .js files and should have a build step with TypeScript to transpile to JS.

[borisdiakur]

Hi @sean-perkins, thanks for picking up my PR 🙏

I'm not sure I understand why these changes are necessary. Ionic Framework uses the output targets to ship to both React and Vue, and our React starters take advantage of create-react-app as well. There has been no reported issues and our starters are functioning as expected.

I think the reason why you do not run into the same issue with your React starters is, that they don't have "type": "module", declared in their package.json file.

The source files you changed are TypeScript files that are copied into the destination project. They are not .js files and should have a build step with TypeScript to transpile to JS.

Yes, we do transpile the ts files in our project. However, without the changes included in this PR, the imports, which end up in the js files, do not include the .js / /index.js suffix and I have not found a way to configure Typescript, so that it adds these explicitly (see microsoft/TypeScript#16577).

So, two things come together and lead to a Module not found error:

1. The generated .js files reside in a folder, that contains a package.json with "type": "module",
2. The app consuming our library is bundled with Webpack which has resolve.fullySpecified set to true by default.

Here is what the Webpack docs say about the resolve.fullySpecified setting:

When enabled, you should provide the file extension when importing a module in .mjs files or any other .js files when their nearest parent package.json file contains a "type" field with a value of "module", otherwise webpack would fail the compiling with a Module not found error. And webpack won't resolve directories with filenames defined in the resolve.mainFiles, you have to specify the filename yourself.

I hope this sheds enough light on the issue, which this PR is trying to solve. Please note, that my PR only solves the described issue for the usage of React bindings and Vue bindings. The issue may still be unresolved for other output targets.

[sean-perkins]

Thanks for the added detail 👍

I'll need to test these changes in more depth against Ionic Framework to make sure there is no unexpected regressions. It is difficult to decipher from that linked issue and other Stackoverflow posts if the .js approach is the intended solution or simply a workaround for the ecosystem issue at large.

I can understand why this would be needed in esm for the JS implementation, I'm not entirely sure why it would be needed in this diff: https://github.com/ionic-team/stencil-ds-output-targets/pull/322/files#diff-73978a26d648053b26c05a5ff77f148e2623d91d13c9d10ffb6b62986aeabb9aR53

Can you explain why it would be necessary for types as well?

[borisdiakur]

I can understand why this would be needed in esm for the JS implementation, I'm not entirely sure why it would be needed in this diff: https://github.com/ionic-team/stencil-ds-output-targets/pull/322/files#diff-73978a26d648053b26c05a5ff77f148e2623d91d13c9d10ffb6b62986aeabb9aR53
Can you explain why it would be necessary for types as well?

Yes, that's a mistake. It's not necessary for types. 😣

[JessicaSachs]

Thank you again for opening this PR. I'm sorry it took so long to get reviewed! I had the time to test out this fix and it looks good to me.

Your fix reveals a larger issue around how the output targets create codegen that should be consumable by user code without errors.

Since node and the browser are shifting away from CJS to MJS (which requires explicit file extensions and does not enable implicit folder imports anymore), the generated code became incompatible with certain user project setups. I'm looking forward to a better project-level testing infrastructure which will allow people to catch issues like this.

Found an issue in the implementation

[JessicaSachs]

The tests aren't passing because they're actually TS files. I need to dig into the build + compile step to fix this - adding .js to the end of the node code isn't the right answer.

I'm aware of the issue now though, and I'll add a ticket in Jira to look into it. Supporting projects with type: module is important 😄

[rayboschbr]

Was there any progress with this?