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

start of ES6 modules: move to rollup #183

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Conversation

alicebob
Copy link
Contributor

This is the minimal version of #178

The build process is still to concat the JS files (as ./dist/js/pzpr.concat.js), but this file is now a ES6 module. This concated file is then transformed for the browser via rollup. The actual code of the module didn't change. The tests do use the modern import ... from ... syntax to load data.

Files are still concatted, but they are now converted to something the
browser understands via `rollup`.
These are now executed in strict context.
Tests need the variant files, which are (still) copied during the build.
Copy link
Owner

@robx robx left a comment

Choose a reason for hiding this comment

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

Mostly a nitpicky review with whatever came to mind while first reading over it. Feel free to ignore parts that don't make sense. Also I still need to have a bit of a look at rollup.

Makefile Outdated Show resolved Hide resolved
package.json Outdated
@@ -21,25 +21,28 @@
"node": ">= 5.6.0"
},
"scripts": {
"build": "eslint --cache --quiet src src-ui && ./git-hash.sh && grunt default",
"build": "./git-hash.sh && grunt default",
Copy link
Owner

Choose a reason for hiding this comment

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

the git-hash part here seems like it should be redundant with the new Makefile target

Copy link
Owner

Choose a reason for hiding this comment

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

could we get rid of this script entirely and call npx grunt default from the Makefile instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p.html and the examples and such are still copied with Grunt. I thought is was a bit too much to also change that in this PR. But we could.

Copy link
Owner

Choose a reason for hiding this comment

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

I didn't mean to get rid of Grunt, I meant to get rid of the "build" script in package.json, and to call Grunt directly from the Makefile.

package.json Outdated
@@ -21,25 +21,28 @@
"node": ">= 5.6.0"
},
"scripts": {
"build": "eslint --cache --quiet src src-ui && ./git-hash.sh && grunt default",
"build": "./git-hash.sh && grunt default",
"release": "npm run clean && eslint --cache --quiet src && grunt release",
Copy link
Owner

Choose a reason for hiding this comment

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

is this (still) used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's referenced in Gruntfile:

  var PRODUCTION = (grunt.cli.tasks.indexOf('release') >= 0);

But the only difference seems to be that if PRODUCTION is true it copies over .map files, which is fine anyway?

Copy link
Owner

Choose a reason for hiding this comment

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

It's at least fine in the sense that I'm currently deploying those anyway... https://puzz.link/js/list.js.map

Makefile Outdated Show resolved Hide resolved
src-ui/list.html Show resolved Hide resolved
src/puzzle/Piece.js Outdated Show resolved Hide resolved
test/.eslintrc.json Outdated Show resolved Hide resolved
@alicebob
Copy link
Contributor Author

@robx thanks for the comments, this should be it.

@robx
Copy link
Owner

robx commented Apr 28, 2020

Having a bit of trouble working with this. Specifically,

$ make build
[...]
npx rollup -c ./rollup.config.js
Unexpected token import
make: *** [rollup] Error 1

Any clue?

Also, we might want to keep Grunt for concatenation, since that's what gives us source maps and thus decent stack traces on test failure. (This works, when run via npx grunt concat:pzpr instead of the bundle Makefile target: e15d00c.)

@robx
Copy link
Owner

robx commented Apr 28, 2020

I've pushed two fixes to master that should obviate the need for those isFreeze checks.

Base automatically changed from master to main February 3, 2021 20:28
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