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

Replace resolve with require.resolve #12

Open
drwpow-figma opened this issue Mar 1, 2025 · 5 comments
Open

Replace resolve with require.resolve #12

drwpow-figma opened this issue Mar 1, 2025 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@drwpow-figma
Copy link

Hello! Thanks for taking on maintenance of this plugin, you’ve done a great job!

Problem

One issue with the original rollup-plugin-styles is the old, outdated module resolution handled by the resolve package. Specifically, it doesn’t respect package.json exports that are essential and standard nowadays.

For example, we have a pnpm monorepo, and in one package we have .css files in the ./dist/ folder that we shorten the import path for:

{
  "exports": {
    "./*.css": "./dist/*.css"
  }
}

So you’d import them at @import "local-pkg/foo.css" instead of @import "local-pkg/dist/foo.css". This plugin gives the following error:

(plugin styles) Error: Failed to find 'local-pkg/foo.css'
  in [
    ~/components/src/button
  ]
src/button/button.css

This is a longstanding problem with resolve, which has fallen behind modern ESM support.

Suggested solution

Simply replacing with Node’s built-in require.resolve() should perform much better, would support all of Node’s new ESM features and resolution. Node’s built-in resolve has come a long way and is now more mature than the old resolve package.

For example, my error comes from utils/resolve.ts, where the following change works:

- const resolverAsync = async (id: string, options: AsyncOpts = {}): Promise<string | undefined> =>
  new Promise(resolve => resolver(id, options, (_, res) => resolve(res)));
+ const resolverSync = (id: string, options: AsyncOpts = {}): string | undefined => {
+   try {
+     return require.resolve(res, { paths: [options.basedir] })
+   } catch {
+     // noop
+   }
+ };

Note: this is just an example; there’d be other related changes, but you get the idea

Would be happy to help with this change if this is desirable 🙂

@megheaiulian megheaiulian self-assigned this Mar 5, 2025
@megheaiulian megheaiulian added the enhancement New feature or request label Mar 5, 2025
@megheaiulian
Copy link

This change is desirable and makes a lot of sense. If you have an idea how to implement it I'd be more than happy to accept a PR.

@drwpow-figma
Copy link
Author

Sure! I can submit a PR shortly after testing it works on our setup

@drwpow-figma
Copy link
Author

One potential blocker: it looks like there is some existing code that uses the following options from resolve:

  • preserveSymlinks: this will basically always be true because Node.js follows symlinks
  • packageFilter: the ability to transform a package.json before reading it would also go away. Node.js doesn’t have this ability, and implementing a half-version may introduce more bugs and not be worth it
  • extensions: Node.js now handles all valid extensions (.js, .cjs, .mjs). The only change would be no automatic .json import by default (but also how used is that behavior?)

All that said, this may likely be a breaking change for folks. I think it would certainly modernize this package! But some of the old behavior may go away. Given all that, do the breaking changes seem like OK tradeoffs?

@megheaiulian
Copy link

What do you think about using enhanced-resolve or unrs/unrs-resolver instead ?

@drwpow-figma
Copy link
Author

What do you think about using enhanced-resolve or unrs/unrs-resolver instead ?

Those are both great options! I didn’t know about those. Can’t really go wrong with either—enhanced-resolve is by some of the webpack team it looks like; unrs-resolver is from the UnJS team that does great work. Neither has the packageFilter, but perhaps the way it’s being used in this library we could simply use the other options available to achieve the same effect (since it’s for the purpose of resolving)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants