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

refactor(kit,nuxi): resolve module paths using node algorithm #19537

Merged
merged 8 commits into from Mar 10, 2023

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Mar 8, 2023

πŸ”— Linked issue

#18431 (comment)

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The linked issue is caused by us trying to resolve an ESM module's path by CJS utilities, and then import it with native ESM.

This refactors our internal CJS utils in kit + nuxi to use resolvePath from mlly which uses import-meta-resolve under the hood (a polyfill for the Node built-in). To decrease the change surface, I've restricted changes to places where we have a single search path (e.g. rootDir or cwd) and have kept the existing behaviour where we have multiple paths (e.g. where nuxt.options.modulesDir is used).

I would be up for refactoring the remaining usage of CJS utils as well, but am also happy to defer that to a subsequent PR.

As part of this, the behaviour of tryResolveModule (exposed by kit) has changed (it is now async, it now returns a file URL, and it takes a single string as a second argument rather than an object). However, I think this is safe as it was a deprecated internal module and not used outside of nuxt itself.

I've also decreased the number of internal utilities we are exposing from the CJS part of kit as a number of them are not being used elsewhere in the repo.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe requested a review from pi0 March 8, 2023 20:54
@danielroe danielroe self-assigned this Mar 8, 2023
@codesandbox
Copy link

codesandbox bot commented Mar 8, 2023

CodeSandbox logoCodeSandbox logoΒ  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@danielroe danielroe requested a review from antfu March 9, 2023 15:09
@pi0
Copy link
Member

pi0 commented Mar 10, 2023

This is a really lovely initiative! Only I think we should use mlly.resolvePath that wraps same package internally for less deps, more consistency and path normalizations.

@danielroe danielroe changed the title refactor(kit,nuxi): use import-meta-resolve to resolve module paths refactor(kit,nuxi): resolve module paths using node resolver algorithm Mar 10, 2023
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good must have to me πŸš€

@danielroe danielroe changed the title refactor(kit,nuxi): resolve module paths using node resolver algorithm refactor(kit,nuxi): resolve module paths using node algorithm Mar 10, 2023
@danielroe danielroe merged commit 6d79b71 into main Mar 10, 2023
@danielroe danielroe deleted the fix/esm-utils branch March 10, 2023 14:55
@danielroe danielroe mentioned this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants