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

Move to ES6 #127

Closed
ShukantPal opened this issue Aug 1, 2020 · 10 comments
Closed

Move to ES6 #127

ShukantPal opened this issue Aug 1, 2020 · 10 comments

Comments

@ShukantPal
Copy link

Are there plans to modernize the codebase, to probably encourage more development? I could make a quick PR for you @Pomax

@Pomax
Copy link
Owner

Pomax commented Aug 1, 2020

Quick PRs tend to just be "ES6ish" in that they're broad stroke replacements that use the new syntax, but don't actually involve significantly rewriting code to take advantage of the patterns that ES2015-ES2020 enables, compared to ES5, so I'd rather not have a "quick PR" and would much more prefer a proper rewrite (which can of course be in the form of lots of small PRs) =)

Having said that, I would love to modernize it, and am currently focused on reworking the Primer's codebase, writing a custom element for the on-page graphics so I can replace the React code that exists for that, to turn it back into a true (but modern) HTML page.

If you want to get a head start on me getting to bezier.js, by all means have at it!

@Pomax
Copy link
Owner

Pomax commented Aug 7, 2020

Based on musings in #128:

If bezier.js gets uplifted to ES6, then that should be fully committed to: no more require, nothing that generates ES5, and ES5 code patterns that have been replaced with better ones in ES6, replaced with those better ones. Also, for linting, let's use https://prettier.io, without any customization: whatever it considers good code is simply what the code will look like (and that can change from version to version, but that's fine), with npm test automatically rewriting things to be prettier-conformant.

Also, let's do this in small, easy to PR, easy to review, and easy to land steps:

  • 1. let's start a new es6 branch, with just the /lib code and the old test file, and uplift those first, with an npm script for automatic linting using prettier (done!)
  • 2. initial super-minimal rewrites just so that things use import/export without any other code changes
  • 3. then, switch things over to normal modern class syntax, without any code changes inside functions
  • 4. then, replace all var with let and const (with aggregation where appropriate) and fixing any use of bind().
    • 1. bezier.js
    • 2. poly-bezier.js
    • 3. utils.js
    • 4. svg-to-beziers.js
    • 5. normalise-svg.js (should be replaced with the latest version from svg-path-reverse)
  • 5. then it's probably a good time to switch the testing to jest, because it's still using the weird ancient hand-written testing atm.
  • 5. (revised) then it's probably a good time to switch the testing to mocha, as Jest does not support ES modules.
  • 6. move the tests from old-tests.js over to the new mocha test dir.
  • 7. then, start rewriting function internals to ES6 syntax and patterns, writing tests as we go, with one PR for each function (or codepath, if the functions tightly rely on each other) so reviews are quick and independent:
    • 1. first write the test (that is, both tests that should pass, and tests that should fail because without that second type of testing, you won't catch an entire class of bugs), which should all pass because the code should already work as intended =P
    • 2. then refactor the code to take advantage of ES6 syntax and patterns
    • 3. then make sure that core still passes testing.
  • 8. refactor the code so so that anything that can be modularised/housed as its own unit, is (this is the very last step, because it is effectively cosmetic wrt. code layout, but we want to do this after everything has tests so that we can verify that moving functions around doesn't break the tests)

Once that work's done, we can add in rollup so that a browser "bundle" can be created. Note that minification is not necessary here: any sane server is going to serve that file with gzip/brotli compression already, which is superior to any kind of JS based minification solution out there, and can actually perform worse on pre-minified code.

@Pomax
Copy link
Owner

Pomax commented Aug 7, 2020

I will probably do some work on this tomorrow (Friday, August 7) too, since I've reached the point in my rewrite of the Bezier Primer that it needs an ES6 version of Bezier.js for all the examples to actually work.

@Pomax
Copy link
Owner

Pomax commented Aug 7, 2020

I've finished step 2 and will be working on step 3.

@Pomax
Copy link
Owner

Pomax commented Aug 7, 2020

I've finished step 3 and will be working on step 4.

Once that's done, we'll be ready to tackle the real "meat and potatoes" of this issue: auditing, writing tests for, and (hopefully) modernizing the code for every single function.

@Pomax
Copy link
Owner

Pomax commented Aug 7, 2020

step 4 down, I'll get step 5/6 out of the way and then probably stop for a bit because step 7 is just going to be a lot of PRs =)

@Pomax
Copy link
Owner

Pomax commented Aug 7, 2020

turns out Jest has some issues with detecting module code, so that's not quite as much of a drop-in as I'd hoped... Jest 27 is supposedly going to have ESM support (if they don't push it again, it was initially slated for Jest 26) but 26 was released in May, so I have low hopes for 27 being released before next year.

@Pomax
Copy link
Owner

Pomax commented Aug 8, 2020

Steps 5 and 6 done, so I'm going to (literally =) call it a day and will probably let this sit for a few days now that I can use bezier.js as an ES module (albeit not a "done" es module) for the purposes of redoing the Primer tech stack and sketch code.

@ShukantPal
Copy link
Author

@Pomax I'll start step 7 this weekend ;)

@Pomax
Copy link
Owner

Pomax commented Oct 24, 2020

ES6 deployed.

@Pomax Pomax closed this as completed Oct 24, 2020
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

No branches or pull requests

2 participants