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

More robust loading of CJS plugins using require() #141

Merged
merged 1 commit into from Nov 17, 2022
Merged

Conversation

bmish
Copy link
Owner

@bmish bmish commented Oct 17, 2022

For CJS plugins, we should just load them with Node's require function. This defers to Node on resolving/loading the package entry point, meaning we don't have to try to handle any of that logic ourselves. As a result, support for CJS plugins should be improved. This greater robustness means any file types (e.g. JSON) handled by require are also supported now.

Only for ESM plugins do we need to manually try to resolve the package entry point to pass to the import function. That's because you can't just do import('path/to/plugin') for importing a local directory. import does not automatically resolve anything, so we have to do attempt to do it ourselves by looking at exports in package.json.

Related to #140. Helps with #132.

@ljharb
Copy link

ljharb commented Oct 30, 2022

It seems like it'd be easier to just author in CJS modules, and require or await import() as needed?

@bmish bmish force-pushed the require-cjs branch 7 times, most recently from 98f844a to 95de208 Compare November 16, 2022 17:00
@bmish bmish changed the title Use require to load CJS plugins Use require() to load CJS plugins Nov 16, 2022
@bmish
Copy link
Owner Author

bmish commented Nov 16, 2022

It seems like it'd be easier to just author in CJS modules, and require or await import() as needed?

@ljharb just to respond to this:

  1. I have the change working. The only problem is that I haven't gotten it to work in the jest tests yet. Making slow progress on the tests dealing with require weirdness in jest.
  2. Authoring this tool in CJS wouldn't help and would actually preclude us from being able to import() ESM plugins (which is essential since the new ESLint config system supports ESM configs).

@ljharb
Copy link

ljharb commented Nov 16, 2022

Why? import(), whether in CJS or ESM, always loads ESM.

@bmish
Copy link
Owner Author

bmish commented Nov 16, 2022

Oh, nice, I think I overlooked how dynamic import can load ESM from CJS. Either way, it's nicer to write code in ESM.

@ljharb
Copy link

ljharb commented Nov 16, 2022

That seems like something that could be addressed with babel without adding the complexity of createRequire, but that's certainly up to you.

@bmish bmish force-pushed the require-cjs branch 3 times, most recently from 151378d to 9af9b1a Compare November 17, 2022 17:12
@bmish bmish marked this pull request as ready for review November 17, 2022 17:23
@bmish bmish changed the title Use require() to load CJS plugins More robust loading of CJS plugins using require() Nov 17, 2022
@bmish bmish added enhancement New feature or request and removed bug Something isn't working labels Nov 17, 2022
@bmish bmish merged commit ae3e411 into main Nov 17, 2022
@bmish bmish deleted the require-cjs branch November 17, 2022 17:32
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

Successfully merging this pull request may close these issues.

None yet

2 participants