-
Notifications
You must be signed in to change notification settings - Fork 115
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
fix: fully specify resolve path with .js extension #322
base: main
Are you sure you want to change the base?
Conversation
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 |
Hi @sean-perkins, thanks for picking up my PR 🙏
I think the reason why you do not run into the same issue with your React starters is, that they don't have
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 So, two things come together and lead to a
Here is what the Webpack docs say about the
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. |
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 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. 😣 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The tests aren't passing because they're actually TS files. I need to dig into the build + compile step to fix this - adding I'm aware of the issue now though, and I'll add a ticket in Jira to look into it. Supporting projects with |
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).
Was there any progress with this? |
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:
npm run build
) was run locally for affected output targetsnpm test
) were run locally and passednpm 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:
What is the current behavior?
In Nuxt 3 I get the following error when using the vue output target:
In a create-react-app I get the following error when using the react output target:
Issue URL: #319
What is the new behavior?
Does this introduce a breaking change?
Other information
See also microsoft/TypeScript#16577