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 #83

Merged
merged 8 commits into from
Mar 4, 2020
Merged

ESM #83

merged 8 commits into from
Mar 4, 2020

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Mar 1, 2020

Builds on #81 and #82.

  • Enhancement: Add ESM distribution pointed to by module
  • Refactoring: Switch to ESM internally
  • Build: Ensure dist files are bundled in npm

Besides being a preparation for supporting Node ESM (this is aimed at browser ESM), using ESM will assist a proposed conversion to Mocha + nyc for coverage (since there seems no longer a need for specialized define testing).

…unignore `.eslintrc.js`;

    use "js" extension for comments; enforce `semi` rule; lint `parser.js` to catch any buggy generator issues
- Linting (package.json): Add recommended fields (contributors, bugs, homepage); update repository URL
- Refactoring: Fix broken or redundant HTML
- Maintenance: Enforce 2 sp. for JSON in `.editorconfig` (per current `package.json`)
- npm: Add build scripts (leading currently to Makefile)
- Refactoring: remove now unneed Array.isArray polyfill
- npm: Update estraverse dep. minor version and devDeps.
…ull` for `?`)

- npm: Add npm scripts in place of Makefile (and remove `parser.js` upon running the parser build)
- npm: Simplify to reference binary without `node_modules`
- Refactoring: Switch to ESM internally
- Build: Ensure `dist` files are bundled in npm
@brettz9
Copy link
Contributor Author

brettz9 commented Mar 1, 2020

I believe this build is failing as dist is not included in the repo, yet the tests require a CommonJS build (which is no longer source, but a dist). However, this PR is more of a stepping stone to #84 (and #85) which are not failing (as we can point back to source with the tests in Mocha + the npm package esm supporting running against the ESM source). If you decide you do not want those PRs for some reason, I could look to including the CommonJS distribution in the repo so it can be used in tests.

@brettz9 brettz9 mentioned this pull request Mar 1, 2020
@michaelficarra
Copy link
Member

This generally LGTM.

@michaelficarra
Copy link
Member

@brettz9 Rollup seems to be choking on default parameters. Does it need some configuration to support modern syntax?

@brettz9
Copy link
Contributor Author

brettz9 commented Mar 4, 2020

Sorry, the issue there is with our using such syntax on an older version of Node. I can change the syntax, but if you are open to the breaking change on the Mocha PR, it will remove those travis versions anyways.

@brettz9
Copy link
Contributor Author

brettz9 commented Mar 4, 2020

However, if you like, I should be able to get it working for Node 4/6 in this PR, and just amend the syntax in the Mocha PR.

@brettz9
Copy link
Contributor Author

brettz9 commented Mar 4, 2020

I tried amending my usage of Rollup, but I see Rollup itself is internally using default params. :-) . So I can either see about using an older version of Rollup, or if you are ok with the breaking change to require Node 8 (as in my Mocha PR also), I can just drop those versions in Travis in this PR.

@michaelficarra
Copy link
Member

Yeah node 4/6 are effectively as old as the other ancient ones we dropped in your earlier PRs. I'll remove them from the travis config and get this merged.

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