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

[MAJOR] js: Move to webpack, pug from r.js, jade #800

Merged
merged 5 commits into from
Mar 20, 2022

Conversation

ix5
Copy link
Member

@ix5 ix5 commented Feb 19, 2022

Introduction

As already discussed in #754, try to make Isso more maintainable by facilitating modern testing practices.

This PR is a bit massive, but trying to make pug or other updated deps play nice with RequireJs just for the sake of a clean history is not something I'm prepared to do.

So, here goes:


js: Move to webpack, pug from r.js, jade

In the interest of maintainability and integration with other testing tools, get rid of RequireJS (sometimes also called r.js or requirejs)

This necessitates the following changes:

  • Removing the almond (AMD) module loader (superseded by webpack internals)
  • Removing requirejs-text for loading static resources (superseded by webpack asset/source loader)
  • Removing jade templating engine (superseded by pug, which represents the original jade project that had to be renamed due to trademark issues)
    pug v2+ no longer accepts 'mystr-{#foo.bar}'-style variables inside attributes, so *.pug templates need to be changed accordingly (mystr' + foo.bar)
    Note: templates have been moved to templates/ directory and renamed from .jade to .pug.
  • jade-runtime and accompanying shims (superseded by simply pug-runtime)
  • Move .svg files inside own dir, simplify loading via webpack's asset/source
  • Add webpack config
  • Add build scripts to package.json
  • Adjust Makefile to use webpack via npm run
  • Emit JS source map files from webpack (eases debugging)
  • .gitignore JS source map files

While RequireJS utilized the following syntax:

define(["app/foo"], function(foo) {
  var bar = doSomeStuff...
  return bar;
})

Jest and other testing libraries need the NodeJS standard CommonJS style:

const foo = require("app/foo");
var bar = doSomeStuff...
module.exports = { bar };

Note: ES6/ECMAScript2015-style import has not been used since it would need transpilation via e.g. babel to be understood by Jest.

The RequireJS-style wrapping of all code inside define({...}) caused everything to be indented by at least one level.
To keep the changes in this commit manageable, indentation will be removed in a later commit.

isso: js: treewide: De-indent one level

Since switching from RequireJS to CommonJS syntax, the define() wrapper is no longer needed and as such, the the code formerly inside parentheses can be de-indented by one level.

isso: js: i18: RequireJS to module.exports

(Just an extension of previous commit, to avoid touching too many files at once)

demo: Use dev version of embed.js

This makes it easier to debug when testing. The demo shouldn't be user-facing, but either way, the performance penalty should be outweighed by debugging benefits.

isso: js: Reduce deps for app/lib/

(Just a minor commit to make the following unit tests easier)


Verifying

You can verify this commit by checking out this PR:

git clone https://github.com/posativ/isso isso
cd isso/
git fetch origin pull/800/head:pull-800
git checkout pull-800

And then installing everything as documented in docs: Install from Source.

Then, either embed Isso into your site or use the demo at localhost:8080/demo


A note for people from the future: You can use git blame --ignore-rev=<sha> to "ignore" the de-indent commits. See A better git blame.

Note to myself: We can probably ditch the templates altogether and simply use plain html with javascript string interpolation. That way, we lose pug, pug-loader and pug-runtime, which would make the bundle significantly smaller.

@ix5 ix5 requested a review from blatinier February 19, 2022 16:32
@ix5
Copy link
Member Author

ix5 commented Feb 19, 2022

Pinging @stefangehn, @blatinier, @Lucas-C and anyone else interested in moving this project forward.

@ix5 ix5 mentioned this pull request Feb 19, 2022
@ix5 ix5 added client (Javascript) client code and CSS feature labels Feb 19, 2022
@ix5 ix5 added this to the 0.13 milestone Feb 19, 2022
@ix5
Copy link
Member Author

ix5 commented Feb 23, 2022

I've created a package for easier testing:
isso-0.12.5-webpack.tar.gz

Copy link
Contributor

@Lucas-C Lucas-C left a comment

Choose a reason for hiding this comment

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

This is great!
Awesome job on moving everything to ES6 modules & webpack O.O
Maybe this should be merged as part of #801 as we would have the added confidence of the new tests?

isso/js/app/lib/identicons.js Show resolved Hide resolved
isso/js/app/templates/comment-loader.pug Show resolved Hide resolved
isso/js/app/templates/comment.pug Show resolved Hide resolved
isso/js/app/template.js Outdated Show resolved Hide resolved
@ix5
Copy link
Member Author

ix5 commented Mar 3, 2022

This is great! Awesome job on moving everything to ES6 modules & webpack

Thank you for reviewing! Those questions you asked were all obstacles I stumbled upon while porting and was kind of too lazy to document. Thanks for keeping me honest ;) I'll document as I have written in individual responses inline.

Maybe this should be merged as part of #801 as we would have the added confidence of the new tests?

While waiting for the test suite to be adequate might sound like a sensible strategy for keeping bugs out, it'll also mean keeping up a massive amount of rebase work and effectively blocking any contribs to the client (or doing a lot of merge conflict work).

So I'll be selfish and merge this webpack/pug change soon-ish, before tests are done. My "justification" for this is that once 0.12.6 is released, we can afford to be open for integrating more major changes.

@ix5 ix5 force-pushed the js-webpack branch 2 times, most recently from eae2a02 to 1590e46 Compare March 7, 2022 15:19
ix5 added 5 commits March 20, 2022 21:06
This makes it easier to debug when testing. The demo
shouldn't be user-facing, but either way, the performance
penalty should be outweighed by debugging benefits.
In the interest of maintainability and integration with
other testing tools, get rid of `RequireJS` (sometimes also
called `r.js` or `requirejs`)
This necessitates the following changes:
- Removing the `almond` (`AMD`) module loader (superseded by
  webpack internals)
- Removing `requirejs-text` for loading static resources
  (superseded by webpack `asset/source` loader)
- Removing `jade` templating engine (superseded by `pug`,
  which represents the original `jade` project that had to
  be renamed due to trademark issues)
  `pug` v2+ no longer accepts `'mystr-{#foo.bar}'`-style
  variables inside attributes, so `*.pug` templates need to
  be changed accordingly (`mystr' + foo.bar`)
  Note: templates have been moved to `templates/` directory
  and renamed from `.jade` to `.pug`.
- `jade-runtime` and accompanying shims (superseded by
  simply `pug-runtime`)
- Move `.svg` files inside own dir, simplify loading via
  `webpack`'s `asset/source`
- Add `webpack` config
- Add build scripts to `package.json`
- Adjust `Makefile` to use `webpack` via `npm run`
- Emit JS source map files from `webpack` (eases debugging)
- `.gitignore` JS source map files

While RequireJS utilized the following syntax:
```
define(["app/foo"], function(foo) {
  var bar = doSomeStuff...
  return bar;
})
```
Jest and other testing libraries need the NodeJS standard
`CommonJS` style:
```
const foo = require("app/foo");
var bar = doSomeStuff...
module.exports = { bar };
```
Note: ES6/ECMAScript2015-style `import` has not been used
since it would need transpilation via e.g. `babel` to be
understood by Jest.

The RequireJS-style wrapping of all code inside
`define({...})` caused everything to be indented by at least
one level. To keep the changes in this commit manageable,
indentation will be removed in a later commit.
Since switching from RequireJS to CommonJS syntax, the
`define()` wrapper is no longer needed and as such, the the
code formerly inside parentheses can be de-indented by one
level.
@ix5 ix5 merged commit 5528273 into isso-comments:master Mar 20, 2022
@ix5 ix5 deleted the js-webpack branch March 20, 2022 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client (Javascript) client code and CSS feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants