feat: Add CommonJS export for parse5 module#418
Conversation
wooorm
left a comment
wooorm
left a comment
There was a problem hiding this comment.
this looks good, can the CI be fixed though?
|
The CI is fixed in #419 |
| "build": "tsc --build packages/* test", | ||
| "build": "npm run build:esm && npm run build:cjs", | ||
| "build:esm": "tsc --build packages/* test", | ||
| "build:cjs": "tsc -p packages/parse5/tsconfig.cjs.json && echo '{\"type\":\"commonjs\"}' > packages/parse5/dist/cjs/package.json", |
There was a problem hiding this comment.
does this make node think dist/cjs/ is its own package?
There was a problem hiding this comment.
This will make require('parse5') work in Node versions with ESM support.
Because the files are plain .js files, Node will look at the closest package.json file to determine if the code is ESM or not. We create a package.json for the CJS code, to make sure requiring it works as expected.
There was a problem hiding this comment.
i get that part, was just wondering what other effects it might have. since you're telling node the cjs path is its own standalone package by giving it its own package manifest. curiosity more than anything, just wondering what other behaviour it might change in tooling.
There was a problem hiding this comment.
I can't think of any side-effects, but definitely something worth keeping in mind for future bug-reports. I adopted the pattern from this article from SenseDeep, which is used in several published (but not very popular) modules.
| ], | ||
| "license": "MIT", | ||
| "main": "dist/index.js", | ||
| "main": "dist/cjs/index.js", |
There was a problem hiding this comment.
wouldn't we still want the default to be ESM? so the only way to reach any commonjs entrypoints is by reading the package exports
There was a problem hiding this comment.
Node versions with ESM support will not read the main property if exports are specified. That means this field is only for non-ESM Node versions, which should use the CJS version.
Bumps [eslint](https://github.com/eslint/eslint) from 8.9.0 to 8.10.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) - [Commits](eslint/eslint@v8.9.0...v8.10.0) --- updated-dependencies: - dependency-name: eslint dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…#88) Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.12.1 to 5.13.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.13.0/packages/parser) --- updated-dependencies: - dependency-name: "@typescript-eslint/parser" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [typescript](https://github.com/Microsoft/TypeScript) from 4.5.5 to 4.6.2. - [Release notes](https://github.com/Microsoft/TypeScript/releases) - [Commits](microsoft/TypeScript@v4.5.5...v4.6.2) --- updated-dependencies: - dependency-name: typescript dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.12.1 to 5.13.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.13.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [dependabot/fetch-metadata](https://github.com/dependabot/fetch-metadata) from 1.2.1 to 1.3.0. - [Release notes](https://github.com/dependabot/fetch-metadata/releases) - [Commits](dependabot/fetch-metadata@v1.2.1...v1.3.0) --- updated-dependencies: - dependency-name: dependabot/fetch-metadata dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Titus <[email protected]>
Rebase snafoo
|
Thanks for the review y'all! |
| "exports": { | ||
| ".": { | ||
| "import": "dist/index.js", | ||
| "require": "dist/cjs/index.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
this breaks rehype/test/parse-error.js in rehypejs/rehype#113
import p5errors from 'parse5/lib/common/error-codes.js'
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './lib/common/error-codes.js' is not defined by "exports" in node_modules/parse5/package.json imported from test/parse-error.js
wontfix in rehype, as ERR is not exported by parse5
→ export as ErrorCodes?
https://nodejs.org/api/esm.html
module files within packages can be accessed by appending a path to the package name unless the package's package.json contains an "exports" field, in which case files within packages can only be accessed via the paths defined in "exports".
There was a problem hiding this comment.
the package exports are right IMO. if we have consumers of the error codes enum, we should probably just export it top-level along with ParserError, possibly as a useful name like you said (ErrorCodes).
i.e. in index.ts, export {ERR as ErrorCodes} or something
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Titus <[email protected]>
4 participants
Fixes #379
This mostly follows the pattern from SenseDeep.
cc @43081j @wooorm