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

Convert package to ESM #35

Closed
9 tasks
nzakas opened this issue May 7, 2021 · 13 comments · Fixed by #39
Closed
9 tasks

Convert package to ESM #35

nzakas opened this issue May 7, 2021 · 13 comments · Fixed by #39

Comments

@nzakas
Copy link
Member

nzakas commented May 7, 2021

To convert this package from CommonJS to ECMAScript modules (ESM):

  • Add type: module to package.json
  • Convert files in lib to ESM
  • Convert files in tests/lib to ESM
  • Upgrade Mocha
  • Create build script in package.json to generate CommonJS file (.cjs) in dist directory (use Rollup)
  • Add dist to .gitignore
  • Run build in prepare script in package.json to ensure build is run at key moments
  • Update package.json so that main points in the CJS file in dist and exports provides both require and import options
  • Update README instructions
@ljharb
Copy link
Contributor

ljharb commented May 7, 2021

You don’t need type:module at all; that’s only if you want .js files to be ESM (which will break back compat and a number of tools). I’d suggest using .mjs for ESM instead.

@nzakas
Copy link
Member Author

nzakas commented May 8, 2021

We want .JS files to be ESM.

@brettz9
Copy link
Contributor

brettz9 commented Jun 20, 2021

Just a note that due to there being a number of dynamic require's (some for the performance benefits of lazy loading but others necessarily so) and with import() being async, a number of methods (config-array-factory.js) will need to end up as async.

@brettz9
Copy link
Contributor

brettz9 commented Jun 20, 2021

Another question raised is whether you want to change the files which export objects like:

module.exports = {
  method1,
  method2
};

...into named exports (as can support tree-shaking) like:

export{
  method1,
  method2
};

...even though one technically could continue to export as a default object like:

export default {
  method1,
  method2
};

@nzakas
Copy link
Member Author

nzakas commented Jun 24, 2021

We can’t change methods to be async because these APIs are frozen. If that’s a blocker for this effort, then so be it. Ultimately this package will be used infrequently once the new config system is in place, so it’s a lower property to convert.

@brettz9
Copy link
Contributor

brettz9 commented Jun 24, 2021

Yeah, ok, appears it will be a blocker then.

@brettz9
Copy link
Contributor

brettz9 commented Jun 24, 2021

Wait, maybe need to take another look as it may be requiring JSON which could be safely read synchronously.

@brettz9
Copy link
Contributor

brettz9 commented Jun 24, 2021

No, it looks like at least _loadParser and _loadPlugin would need to eval the contents.

@nzakas
Copy link
Member Author

nzakas commented Jun 25, 2021

Could we use createRequire in those cases?

@brettz9
Copy link
Contributor

brettz9 commented Jun 25, 2021

Oh, wonderful. I had just associated that mentally as a workaround for JSON. :-) Will see if I can complete the work then.

@brettz9
Copy link
Contributor

brettz9 commented Jun 25, 2021

This would presumably mean that the parser and plugins could not be expressed themselves (without a build step) as ESM yet though.

@brettz9
Copy link
Contributor

brettz9 commented Jun 25, 2021

For the cases where some require statements could be expressed as import but which are currently not at the top-level and lazy-loaded for purposes of performance, would you like me to continue to use require and just trust that the projects (js-yaml and espree) will not be abandoning a CJS version anytime soon, or express them as top-level imports?

@nzakas
Copy link
Member Author

nzakas commented Jun 25, 2021

Presumably no dependencies would eliminate CJS without a major version change, so I think we are safe using require. (As previously mentioned, these APIs are frozen so we won't be doing any upgrades of dependencies.)

I think it's probably safe to use import for anything that can be statically referenced and just use require for anything dynamic.

@nzakas nzakas closed this as completed in #39 Aug 4, 2021
nzakas pushed a commit that referenced this issue Aug 4, 2021
* Breaking: Switch to ESM

* Fix: CJS changes

* Fix: Though tests in prev. commit passed, change all CJS to cjs extension, except tests/fixtures/rules/make-syntax-error-rule.js given that ModuleResolve uses the Node Resolution Algorithm

* Fix: Rollup did not like requiring of default module.exports (without a plugin at least), so create require

* New: Add exports; also update devDeps. specify external modules in Rollup config, and align it with eslint-scope

* Fix: Add `dist` file

* Fix: We can use cjs for our final file after all

* Test: CommonJS

* Refactor: Drop Node overrides while still using non-ESM config

* Refactor: Drop `Module.createRequireFromPath`

* Test: Supply Node version which won't fail with dev.

* Fix: Windows needs file: for absolute URLs

* Refactor: Remove line break

* Fix: Convert further paths to file: imports for Windows

* Fix: Point `main` to CJS for older browsers

* Chore: update devDeps

* Update .github/workflows/ci.yml

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update tests/_utils/index.js

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Update package.json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* Refactor: Switch two of the `conf` files to ESM

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@nzakas nzakas moved this to Complete in Triage Jan 3, 2023
@nzakas nzakas added this to Triage Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants