- Notifications
You must be signed in to change notification settings - Fork335
Eslint + npm#410
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Eslint + npm#410
Uh oh!
There was an error while loading.Please reload this page.
Conversation
michaelficarra commentedApr 27, 2020
I really don't want to introduce browserify. I'd much prefer switching to node modules and using rollup. |
Uh oh!
There was an error while loading.Please reload this page.
michaelficarra commentedMay 4, 2020
@brettz9 This PR does too many things to do a proper review. Can you break it down into smaller PRs? You don't need one per commit, but please at least have everything in each PR related. |
ad28bf2 to4942c44Comparebrettz9 commentedMay 6, 2020
I've cut back to avoid a lot of the linting changes (which I can add to a subsequent PR). There are still a number of different items in the PR, but I think it should be a lot easier to review now. Let me know if you need it subdivided further. Most of the changes should be related now to updating the deps and some minor packaging additions (metadata or added config files). While I know you weren't keen on supporting browserify elsewhere, commonjs-everywhere was not working with the current set-up (complaining about a reserved word in the updated |
sanex3339 commentedJun 22, 2020
It will be nice to get this merged |
package.json Outdated
| ], | ||
| "version":"1.14.3", | ||
| "engines": { | ||
| "node":">=4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"node": ">=10.0" should be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perestools/esquery#96 (comment) and context,@michaelficarra was inclined for the project to keepengines and only bump if the project is known to fail. I was planning on also adding a PR for babel support on top of this as should allow that backward support (but had been informed there was too much in this PR originally to include the Rollup/Babel routine). So I can bumpengines if desired, or add Babel to provide backward-support, but if the latter, I presume Babel is still desired in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Btw, I've rebased on master, and also noted in my intro comment that I've also added these changes on top of the previously mentioned ones:
- npm: Restore optionator to a regular dep. (used in published binary file)
- npm: Drop unused semver, minimist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
source-map 0.7.x drops support for Node < 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, butsource-map has been a part ofoptionalDependencies.
- Update: Use new SourceMapConsumer API in test- Build: Provide browserified builds with npm package- Travis: Drop 4, 6, 8; Add 10, 12, 14; check build- Maintenance: Add `.editorconfig`- Docs: Use fenced code blocks in README (for syntax highlighting)- npm: Add `bugs`, `keywords`, change from `maintainers` to `authors`/`contributors`- npm: Restore `optionator` to a regular dep. (used in published binary file)- npm: Drop unused semver, minimist- npm: Bump deps. (estraverse, optionator, optional source-map potentially breaking) and devDeps.- npm: Drop bluebird in favor of ES Promises- npm: Use more recently maintained browserify + uglifyify- npm: Replace linting and testing scripts in Gulpfile with npm scripts
- npm: Update package-lock version
- npm: update devDeps.- npm: Update package-lock version
brettz9 commentedFeb 23, 2021
Hi, Anything else you need for this? I've got another PR for Rollup/Babel lined up after this, but keeping it more focused for this PR... |
brettz9 commentedFeb 27, 2022
@michaelficarra : Might I get a review for this? Would like to supply a native ESM/Rollup PR after this if this is ok. |
Uh oh!
There was an error while loading.Please reload this page.
.editorconfigbugs,keywords, change frommaintainerstoauthors/contributorsoptionatorto a regular dep. (used in published binary file)FWIW, I've added a commit for adding a babel routine against the new code in case any of the syntax was not fully supported back to Node 4, though I can undo that if final testing determines it is not necessary.(Planning to add to a later PR)