Skip to content
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

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

benasher44
Copy link
Contributor

@benasher44 benasher44 commented Apr 30, 2023

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

  • Updated an eslint rule to ensure these changes will stick going forward
  • Used no-restricted-imports to specifically target '..' and '.' imports, which weren't covered by import/extensions for some reason.
  • Generally speaking, I think this should work fine with CJS and improve compatibility for TS folks migrating to ESM. I'm less familiar with other supported packaging/module models.
  • In cases where Vite/Rollup didn't like .js extensions for .ts files when building demos, I used .ts endings.

Checklist

  • The changes are not breaking the editor
  • Added tests where possible
  • Followed the guidelines
  • Fixed linting issues

Related issues

Closes #3488

@netlify
Copy link

netlify bot commented Apr 30, 2023

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 7605918
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/64979daebf5d6100080b151c
😎 Deploy Preview https://deploy-preview-4001--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@benasher44 benasher44 force-pushed the basher/import-extensions branch 2 times, most recently from 80678d5 to cacc148 Compare April 30, 2023 20:13
@benasher44 benasher44 changed the title Require .js endings for imports and exports Require file extensions for imports and exports Apr 30, 2023
@benasher44
Copy link
Contributor Author

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

@benasher44
Copy link
Contributor Author

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.

@benasher44
Copy link
Contributor Author

benasher44 commented Jun 18, 2023

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.

@bdbch
Copy link
Contributor

bdbch commented Jun 22, 2023

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

@benasher44
Copy link
Contributor Author

benasher44 commented Jun 22, 2023

Amazing, thank you! I have updated with develop. I'll watch CI to make sure it passes.

@Kyderman
Copy link

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!)

@benasher44
Copy link
Contributor Author

It looks like 1 workflow still has to be approved to run

@benasher44
Copy link
Contributor Author

benasher44 commented Jun 25, 2023

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? 🤞

@bdbch
Copy link
Contributor

bdbch commented Jun 25, 2023

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'],
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@benasher44 benasher44 Jun 25, 2023

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:

  1. Update main tsconfig.json moduleResolution to "NodeNext"
  2. Override moduleResolution in tests/tsconfig.json to be "node", so cypress tests still build/run
  3. Update all prosemirror packages to get the compatible dependencies

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

@benasher44 benasher44 Jan 3, 2024

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!

@svenadlung
Copy link
Contributor

Sorry for the delay on my part. Let's give it a try. Thank you for the contribution @benasher44 🙌

@bdbch bdbch merged commit e97630c into ueberdosis:develop Jun 30, 2023
@benasher44
Copy link
Contributor Author

Amazing, thank you!

@benasher44
Copy link
Contributor Author

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

@benasher44 benasher44 deleted the basher/import-extensions branch June 30, 2023 19:46
andrewlu0 pushed a commit to trybaseplate/tiptap that referenced this pull request Oct 20, 2023
* Require .js endings

* add extension alias for cypress to resolve ts files with js endings
sjdemartini added a commit to sjdemartini/mui-tiptap that referenced this pull request Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type imports broken in TypeScript's NodeNext mode (ESM support)
4 participants