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

use ES6 modules #178

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

use ES6 modules #178

wants to merge 27 commits into from

Conversation

alicebob
Copy link
Contributor

@alicebob alicebob commented Apr 12, 2020

This changes the pzpr module to hang together using import ... from ... (the modern way to do imports nowadays, it seems).

Previously the pzpr module was build by concating all files together, and then use Grunt to uglify the result. Now there is a single /src/pzpr.js which imports all required modules. It's converted to something the browser can use via rollup. The pzpr.js file still builds a global pzpr object, since the UI module and rules.html expect that. The idea is that nothing changes in the final JS file.

You can kinda use this directly from nodejs now, but support for the ES6 modules seems iffy. mocha uses the esm module which works nicely.

notes

  • All variants must still be ported. I only did 2 of them so far. Until then tests will break edit: done, and tests pass
  • I didn't expand the git.version string in pzpr/core.js yet
  • rollup can do "chunks", if you prefer smaller files over something huge.
  • modules are in 'strict' mode, which gave some problems with code changing "frozen" objects (which is allowed, but ignored, in non-strict mode). c0158cb deals with that
  • I had to upgrade travis to use a less ancient node version. I used 10 because that's what I have locally
  • I didn't run prettify, since it'll mess up almost every file

deploy

  • I made the rollup action a makefile target, but there is a rolllup grunt module, so that could be used instead (I had some troubles with it, will try again later) edit: I tried again and fails. Makefile it is.
  • All variants code is now bundled right in the main pzpr.js, so there is no need to copy them in dist
  • Somewhere along the line the candle file doesn't get bundled, so that's an explicit copy to ./dist/ now

ES6 modules crash course

Either there are multiple things exported:

var color = "green";
var foo = "bar";
export {color: color, foo: foo}
import {foo} from './library.js'

Or there is a single thing:

var colors = ["green", "red", "infra"];
export default colors
import colors from './colors.js';

Evaluation

I do think using the ES6 modules is a good goal. It's a nice modernization step. It's unfortunately way too many changes for a single PR. What I can do is break this up in smaller PRs, which would still use some concatenation, and then work towards the state in this PR in a few separate, smaller, steps.

import "./variety-common/Answer.js";
import "./variety-common/BoardExec.js";
import "./variety-common/Encode.js";
import "./variety-common/FileData.js";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these change common in the classmgr. It's likely nicer to do this differently, but it works for now.

var userlang = pzpr.env.node
? process.env.LANG
var userlang = env.node
? "XXX" // process.env.LANG
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one I have no idea how to fix. There is no process variable when using rollup, which is something which comes from nodejs.

@alicebob alicebob marked this pull request as draft April 12, 2020 10:24
@alicebob alicebob marked this pull request as ready for review April 12, 2020 16:07
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.

1 participant