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

Esm eslint #34

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Esm eslint #34

wants to merge 6 commits into from

Conversation

brettz9
Copy link

@brettz9 brettz9 commented Apr 29, 2020

Builds on #33.

  • Breaking change: Enhancement: Add native ESM distribution, with exports property in package.json
  • Refactoring: Add Rollup/Babel/Terser
  • Build: Have generator file produce consumable source files directly rather than logging to console
  • Linting: Switch from JSHint to ESLint (using rules, e.g., indent and prefer-const, in place on other estools project)
  • Testing: Convert coffeescript test files to ES6
  • Testing: Add nyc
  • Testing: Check unmatched high surrogates and full coverage for AST
    expressions (further true isExpression and isStatement's and
    false isProblematicIfStatement), bringing to 100% coverage
  • Travis: Drop previous versions for 12, 14, 16

- Linting: As per latest jshint (define function before called)
- Testing: Use chai's `register-expect`
- npm: Update devDeps (including moving from `coffee-script` to non-deprecated `coffeescript`)
- npm: Simplify script running
- npm: Avoid adding `package-lock.json`
- npm: Update mocha per latest API
- npm: Add recommended package.json fields: keywords, bugs, dependencies and change to author/contributors
- Maintenance: Add `.editorconfig`
@brettz9 brettz9 force-pushed the esm-eslint branch 3 times, most recently from d7b8a33 to 60f4b11 Compare April 29, 2020 12:06
…ge.json`

- Refactoring: Add Rollup/Babel/Terser
- Build: Have generator file produce consumable source files directly rather than logging to console
- Linting: Switch from JSHint to ESLint (using rules, e.g., `indent` and `prefer-const`, in place on other estools project)
- Testing: Convert coffeescript test files to ES6
- Testing: Add nyc
- Testing: Check unmatched high surrogates and full coverage for AST
    expressions (further true `isExpression` and `isStatement`'s and
    false `isProblematicIfStatement`), bringing to 100% coverage
- Travis: Drop previous versions for 10, 12, 14
- Docs: Use heading nesting consistently; rmv trailing spaces
},
"files": [
"LICENSE.BSD",
"README.md",
"lib"
"dist"
Copy link

Choose a reason for hiding this comment

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

this is a breaking change, since the dir name changed.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I thought I would put it out there as such (in case other breaking changes were to be agreed upon anyways, such as use of exports as you mention). I can switch back to lib if the project maintainers wish, especially if there are no other breaking changes like exports being introduced, as would especially justify a switch to the clearer use of dist and which could, going forward, make it easier to avoid breaking changes (except to those using explicit node_modules paths, though even the docs themselves don't seem to consider that a semver violation).

package.json Outdated
"main": "lib/utils.js",
"bugs": "https://github.com/estools/esutils/issues",
"main": "dist/esutils.min.js",
"module": "dist/esutils.esm.min.js",
Copy link

@ljharb ljharb Apr 29, 2020

Choose a reason for hiding this comment

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

this isn’t a standard thing; ESM uses .mjs and the “exports” field (adding that field is also a breaking change)

Copy link
Author

@brettz9 brettz9 Apr 30, 2020

Choose a reason for hiding this comment

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

Yes, it is not standard but it is widely used and supported by bundlers like Webpack and Rollup.

As far as adding that field being a breaking change (if you mean module instead of exports), I'm not sure that would be the case here. It can be a breaking change when the module is pointing to untranspiled source.

There are advantages to pointing to untranspiled source, such as potentially minimizing package size and avoiding baking in dependencies, but that is not the case here where I am pointing to the rolled up (and Babelified) dist file.

And for those who might argue for changing to point to untranspiled source, on the one hand, this has disadvantages, besides of being a breaking change, of putting the work of building (and understanding of how to do so) on the consumer). Still, some might desire such a build, as with estools/esquery#104 just filed against esquery, and for such use cases, it may indeed be better to add the breaking change to untranspiled source since it would bring the above-mentioned benefits (and those wishing to use a separate prebuilt distribution file could point to that file directly, whether the path to it is changed to be modulated by exports or not).

I would be happy adding the "mjs" extension to give proper Node ESM support (though we might clarify that it is not ESM that necessarily uses the extension, but Node which uses it), or a type change and/or the addition of exports (which would itself be a breaking change of sorts).

Copy link

Choose a reason for hiding this comment

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

Adding module isn't a breaking change because nothing should be using nonstandard fields; adding exports is what I was referring to.

Copy link
Author

Choose a reason for hiding this comment

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

As far as the statement that "nothing should be using nonstandard fields", this is a good goal when equivalent ones are available, but otherwise I don't see that as a universally agreed upon goal. :)

module is not necessarily for Node only, nor is need for it removed with standard ESM implementations (it is an entry point for bundling). One might even argue it is a standard relative to the tools that have adopted its usage. And as far as its use relative to Node, Node has even made explicit it not opposing non-Node uses within its package.json, e.g.: re: exports custom conditions:

Other conditions such as "browser", "electron", "deno", "react-native", etc. are ignored by Node.js but may be used by other runtimes or tools. Further restrictions, definitions or guidance on condition names may be provided in the future.

https://nodejs.org/api/esm.html#esm_conditional_exports

But again, I'm happy to add proper Node support for non-bundled usage as well (if there is agreement on using mjs over type: module).

(As an aside re: standards, we might consider that Node itself drifted away from the then CommonJS standard it had been following.)

@ljharb
Copy link

ljharb commented Apr 29, 2020

Rather than using rollup, why not use “exports” which prevents require or import of files you don’t want to be in your public api?

@brettz9
Copy link
Author

brettz9 commented Apr 30, 2020

As you may have been indicating yourself, exports is itself a breaking change itself of sorts, given how it may restrict access to formerly accessible module-based paths.

I like adding Rollup as it offers the ability to provide a version which can work in ESM-supporting browsers without build steps. I'd personally be cool adding exports in addition, having the potential to minimize future breaking changes.

@brettz9
Copy link
Author

brettz9 commented Apr 30, 2020

Btw, though it doesn't look like it should be an issue in this repo, and shouldn't be too much of a big deal in the other estools repos (except impacting instanceof checks for exported classes), there is a potential concern for the "Dual Package Hazard" in estraverse (which has non-isolated state in its configuration of Syntax, VisitorKeys, and VisitorOption) and escodegen (which has non-isolated state for its options).

This might call for its own breaking change to isolate state (preferably the ESM-based approach indicated at https://nodejs.org/api/esm.html#esm_approach_2_isolate_state ).

@ljharb
Copy link

ljharb commented Apr 30, 2020

"work in ESM-supporting browsers without build steps" isn't a universally agreed upon goal, nor one that's pleasant to support in node packages :-/ i'll step back and leave it to maintainers decide if that's something that they think is important.

@brettz9
Copy link
Author

brettz9 commented May 1, 2020

I can also step back to see what the maintainers say about the inclusion of a prebuilt ESM build for browsers, but I will point out:

  1. esquery already accepted a prebuilt ESM build for browsers.
  2. Another estools project, escodegen, already has a browser build, so these are apparently not intended as exclusively Node packages.

BREAKING CHANGE:

limits via `exports`
@brettz9
Copy link
Author

brettz9 commented Apr 7, 2022

I went ahead and added the native ESM support with exports.

If this organization is dead the new releases, I would appreciate a heads up, as I've already started making forks of other projects, not having heard back. But I'm happy to submit my test improvements, etc. if someone still wishes to review and maintain.

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.

2 participants