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

Rewrite to ESM, simplify build process (npm scripts instead of Grunt) #132

Merged
merged 5 commits into from
Apr 3, 2021

Conversation

LeaVerou
Copy link
Collaborator

@LeaVerou LeaVerou commented Apr 2, 2021

This is a fairly substantial PR, but diffs look larger than they are due to whitespace (since no IIFE was needed anymore, the entire source was un-indented), please review with ?w=1.
I also started removing some of the micro-optimizations I mentioned in #131.

All tests still pass (obvs).

The build process is now using npm scripts instead of Grunt/Gulp and the like and consists of:

  • Rollup, for ESM bundling/conversion to other module types, minification (with Terser plugin), replacing the version number (with replace plugin). Previously used Uglify, and there was no bundling or conversion to other module types.
  • Docco used directly, for the annotated source. Previously used a Grunt+Docco plugin.
  • node-qunit-puppeteer for running QUnit via the command line. Previously used a Grunt plugin which also used Puppeteer. I'm also using http-server to start a small local server as Puppeteer seemed to be having issues with running QUnit over file:///. Not sure if we can avoid it or how it was avoided previously (perhaps the Grunt plugin did something like that internally?).

Things I have not done, but either I plan to, or I can if needed:

  • I have not added a watcher, since Rollup has its own, and I guess that's what we really need regenerated as the source changes, but I can if needed.
  • I saw something in the Gruntfile about concatenating 'test/unit_tests.js' and 'test/unit_tests/*.js' but I don't see any unit_tests directory in test?
  • The compress task to create the zip files. Since that's only needed before a release, I figured I could keep this PR smaller and send it earlier). Also, given that Github auto-generates zip files for every release, why do we need this?
  • JS Hint. I was wondering if we could discuss using ESLint instead, which is far more flexible and configurable (e.g. see https://medium.com/@sheldonled/from-jshint-to-eslint-8a0a135fa2bf )

I left in the previous build process as well, but I can remove it in a future PR.

Please let me know if there's anything else I forgot!

Diffs look larger than they are due to whitespace.
Build tools still need to be updated.
@EricSmekens
Copy link
Owner

Answers on your questions that could be done:

  • Could be an improvement for the development process, but I feel like that is not necessary for this PR and can be done later.
  • Probably a leftover from earlier changes/migrations that hasn't been cleaned correctly, so we can remove this if there are no tests there.
  • I think this also is in a 2014 mindset, and using Github's zipping for release if somebody needs to download it without npm, is perfectly fine! So need need to think about it.
  • Same as 1, would be a good improvement, as ESLint is way more supported as well, but that's for another PR as well I think.

@EricSmekens
Copy link
Owner

Great PR, great improvement. Also thank you for the commentary provided with the PR, makes it really clear.

Do you have any steps in mind to taking jsep to a next level?
Lately, I'm not using the library myself, so I might be lacking some vision in where we wish to go.

Going for more extensibility? (In my mind is the open #123, and the Object literal you've mentioned as well).

(I was also thinking about some benchmark-tests, not because they might be necessary, but because I'm interested).

@LeaVerou
Copy link
Collaborator Author

LeaVerou commented Apr 2, 2021

Glad you like it, and thanks for the quick review! Should I merge and send a separate PR for ESLint?

Do you have any steps in mind to taking jsep to a next level?
Lately, I'm not using the library myself, so I might be lacking some vision in where we wish to go.

Going for more extensibility?

Yes, absolutely. I think extensibility should be a top priority right now, and it's something I've wanted to do for a while but never got around to until #123 gave me the nudge I needed :) .

@LeaVerou
Copy link
Collaborator Author

LeaVerou commented Apr 2, 2021

Oh also, should I remove the old build tools?

@EricSmekens
Copy link
Owner

Yes, that is fine. Let's move forward. (Next release will be a biiger one than usual anyway, so let's gid rid of things that we move away from).

@LeaVerou LeaVerou merged commit f9246a4 into master Apr 3, 2021
@LeaVerou LeaVerou deleted the esm branch April 3, 2021 18:37
@LeaVerou
Copy link
Collaborator Author

LeaVerou commented Apr 3, 2021

Yes, that is fine. Let's move forward. (Next release will be a biiger one than usual anyway, so let's gid rid of things that we move away from).

Great, done.

[Bb]uild/
dist/
Copy link
Collaborator

Choose a reason for hiding this comment

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

@LeaVerou - I wonder if the build should be moved back to the build folder (instead of dist) for better compatibility with existing versions? (Could be part of #152 ). I just saw a comment in donmccurdy/expression-eval#41 that made me wonder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dist is convention though.
What if we add a file to build that prints out something like "JSEP has moved to using dist instead of build. Please use dist instead"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we could probably just note it in the changelog?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should definitely note it in the changelog!

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.

None yet

3 participants