-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Require file extensions for imports and exports #4001
Require file extensions for imports and exports #4001
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
80678d5
to
cacc148
Compare
cacc148
to
9f589ce
Compare
In my latest update, I did a more comprehensive test build again our internal app and found a final issue that wasn't caught by the updated eslint rule. I filed it as a bug in import-js/eslint-plugin-import#2777 and added a no-restricted-imports rule to cover the imports from ".." and "." case |
Hi @bdbch! Any thoughts here? prosemirror also recently published nodenext-compatible updates as well, so with this, we should be all set in that area. |
Hi @bdbch. Is this solution okay, or were you looking for something different (will keep this PR updated, if this solution works)? Would love to get this merged. This is the only library we need to fix in order to migrate our node backend to ESM. |
Hey @benasher44, sorry for the late update. I'll give your PR a try later this day and will merge if it works! Wanted to do this too earlier to bring in support for NodeNext. Thanks already for contributing and keeping me in the loop |
9f589ce
to
105d701
Compare
Amazing, thank you! I have updated with develop. I'll watch CI to make sure it passes. |
Just to add to this, I think I'm waiting on this to go in as I can't resolve the imports properly, which is a blocker to implementing TipTap in our more legacy React product (works fine with Next.js!) |
It looks like 1 workflow still has to be approved to run |
105d701
to
7605918
Compare
Okay I added an extensionAlias to the webpack config that cypress is using, so it should properly understand/resolve the .js endings now (all passing for me locally). I also updated with develop again. Mind approving the workflow run once more? 🤞 |
Thanks @benasher44! Let's see what the CI is doing - afterwards the PR looks good for me. I'll let @svenadlung also take a look. |
@@ -43,7 +43,21 @@ module.exports = { | |||
'no-console': ['warn', { allow: ['warn', 'error'] }], | |||
semi: ['error', 'never'], | |||
'import/order': 'off', | |||
'import/extensions': 'off', | |||
'import/extensions': ['error', 'ignorePackages'], |
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.
Do you know if this is autofixeable?
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.
I don't think it is. The only way to get something like that is to convert all of the packages to use moduleResolution: NodeNext, and then TypeScript will suggest imports with .js endings from the start.
I can try that, if you all are open to it. It's a bigger change / commitment.
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.
I briefly tried locally, and it creates a bunch of TS type resolution issues that need to be resolved. The project builds, but there are lots of type issues that appear in the editor.
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.
If you attempt that after merging this, see ProseMirror/prosemirror#1305, where we discussed issues here in depth. In order to build with NodeNext, here's what I did to start:
- Update main tsconfig.json moduleResolution to "NodeNext"
- Override moduleResolution in tests/tsconfig.json to be "node", so cypress tests still build/run
- Update all prosemirror packages to get the compatible dependencies
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.
@bdbch is auto-fixability a blocker?
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.
Not really. I just need another review from @svenadlung since we share PR responsibilities.
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.
Heads up that there is now the "typescript.preferences.importModuleSpecifierEnding": "js"
for VSCode, which will make .js endings the default when auto-importing!
Sorry for the delay on my part. Let's give it a try. Thank you for the contribution @benasher44 🙌 |
Amazing, thank you! |
I did a quick pass on a handful of open PRs in the last week and left comments, but expect to maybe have to quickly follow up with fixes on develop, if people aren't up-to-date with develop |
* Require .js endings * add extension alias for cypress to resolve ts files with js endings
…er ESM This is similar to Tiptap ended up doing here ueberdosis/tiptap#4001.
Please describe your changes
Updates all imports and exports to include file extensions.
How did you accomplish your changes
Careful find/replace, eslint, and iterations of building and fixing issues.
How have you tested your changes
Built the project and demos, tested the changes with our main project, and tried a few examples in the deployed demos.
How can we verify your changes
I'm not really sure how you all test something like this, but it sounded like from a comment in #3488 there was some method in mind. Let me know how I can help here.
Remarks
import/extensions
for some reason.Checklist
Related issues
Closes #3488