Skip to content

Conversation

DavidOliver
Copy link
Contributor

@DavidOliver DavidOliver commented Sep 13, 2022

Regarding #1542

Please note that when the formatters directory is updated with the results of the new build process, the formatters/html/fonts directory should be deleted as the font file is now placed in the dist directory along with the JS and CSS.


I didn't get anywhere with the two available esbuild Handlebars loaders, so the assets build script instead precompiles the Handlebars templates.

The HTML formatter's digest function has been updated to produce 8- character uppercase hashes in order to match those generated by esbuild.

The ESLint config was re-initiated and now uses its default parser (Espree), so Babel is no longer required.

For the Elixir-based tests to result in up-to-date build dist contents I found I had to empty the _build directory.

Karma-run tests

I found that simply swapping out webpack for esbuild in karma.conf.js resulted in all tests continuing to pass. I haven't provided any advanced configuration or details of build config for esbuild here, which I was (erroneously?) expecting I'd have to. 🚨 Please check you're happy with this config before merging. 🚨

npm packages

Before

added 1189 packages, and audited 1190 packages in 5s

25 vulnerabilities (14 moderate, 8 high, 3 critical)

After

added 369 packages, and audited 370 packages in 2s

found 0 vulnerabilities

Build task

Before

$ npm run build

> build
> webpack --config ./webpack.js

ℹ 「webpack」: Starting Build
ℹ 「webpack」: Build Finished

webpack v4.46.0

6b6bcb56ee1345899278
  size     name         module                                                        status
  6.11 kB  0            ./js/helpers.js                                               built
  996 B    5            ./js/makeup.js                                                built
  16 kB    14           ./js/handlebars/templates/sidebar-items.handlebars            built
  2.69 kB  16           ./js/handlebars/templates/versions-dropdown.handlebars        built
  7.69 kB  17           ./js/handlebars/templates/search-results.handlebars           built
  623 B    18           ./js/handlebars/templates/modal-layout.handlebars             built
  630 B    19           ./js/handlebars/templates/quick-switch-modal-body.handlebars  built
  2.12 kB  20           ./js/handlebars/templates/quick-switch-results.handlebars     built
  5.12 kB  21           ./js/handlebars/templates/settings-modal-body.handlebars      built
  67 B     24           ./js/entry/epub.js                                            built
  39 B     25           ./css/entry/epub-elixir.css                                   built
  39 B     26           ./css/entry/epub-erlang.css                                   built
  39 B     47           ./css/entry/html-elixir.css                                   built
  39 B     48           ./css/entry/html-erlang.css                                   built
  86.3 kB  49           ./js/entry/html.js + 20 modules                               built

  size     name         asset                                                         status
  4.11 kB  epub-app     epub/dist/app-695ab404a17935043e9c.js                         emitted
  936 B    epub-elixir  epub/dist/elixir-4e188dafb9c1875883d6.js                      emitted
  8.69 kB  epub-elixir  epub/dist/elixir-75e017cfaa8cd141f98b.css                     emitted
  8.69 kB  epub-erlang  epub/dist/erlang-6290aebe8639c9bf9245.css                     emitted
  936 B    epub-erlang  epub/dist/erlang-f114381bcb19c66599a1.js                      emitted
  111 kB   html-app     html/dist/app-e26a2b5166bc4ff5ee2a.js                         emitted
  956 B    txt          html/dist/app-e26a2b5166bc4ff5ee2a.js.LICENSE.txt             emitted
  936 B    html-elixir  html/dist/elixir-2d4eeb9b042db684c180.js                      emitted
  38 kB    html-elixir  html/dist/elixir-9ce7dd46a4899d642da8.css                     emitted
  936 B    html-erlang  html/dist/erlang-106e89c2dc3b9f6ee80d.js                      emitted
  38 kB    html-erlang  html/dist/erlang-4829bb461d555c496ec7.css                     emitted
  1.82 kB  woff2        html/fonts/remixicon.woff2                                    emitted

  Δt 1909ms (70 modules hidden)

After

$ npm run build

> build
> node build/build.js


  ../formatters/epub/dist/epub-elixir-ZJU2SYWT.css  9.0kb
  ../formatters/epub/dist/epub-erlang-HSD3HX4M.css  9.0kb
  ../formatters/epub/dist/epub-75RCTLK3.js          392b

⚡ Done in 6ms

  ../formatters/html/dist/handlebars.runtime-NWIB6V2M.js  19.9kb

⚡ Done in 14ms

  ../formatters/html/dist/html-FVR7JTYI.js                  58.4kb
  ../formatters/html/dist/html-elixir-BZSPL3VY.css          37.9kb
  ../formatters/html/dist/html-erlang-5NGA25XN.css          37.9kb
  ../formatters/html/dist/handlebars.templates-X7YVL3G2.js  25.1kb
  ../formatters/html/dist/remixicon-E5UFLALU.woff2           1.8kb

⚡ Done in 20ms

Build results

Before

$ tree -h formatters/html
formatters/html
├── [4.0K]  dist
│   ├── [108K]  app-e26a2b5166bc4ff5ee2a.js
│   ├── [ 956]  app-e26a2b5166bc4ff5ee2a.js.LICENSE.txt
│   ├── [ 37K]  elixir-9ce7dd46a4899d642da8.css
│   └── [ 37K]  erlang-4829bb461d555c496ec7.css
└── [4.0K]  fonts
    └── [1.8K]  remixicon.woff2

After

$ tree -h formatters/html
formatters/html
└── [4.0K]  dist
    ├── [ 20K]  handlebars.runtime-NWIB6V2M.js
    ├── [ 25K]  handlebars.templates-X7YVL3G2.js
    ├── [ 38K]  html-elixir-BZSPL3VY.css
    ├── [ 38K]  html-erlang-5NGA25XN.css
    ├── [ 58K]  html-FVR7JTYI.js
    └── [1.8K]  remixicon-E5UFLALU.woff2

I didn't get anywhere with the two available esbuild Handlebars loaders,
so the assets build script instead precompiles the Handlebars templates.
https://handlebarsjs.com/installation/precompilation.html

The HTML formatter's digest function has been updated to produce 8-
character uppercase hashes in order to match those generated by esbuild.

The ESLint config was re-initiated and now uses its default parser
(Espree), so Babel is no longer required.

This commit does not update the formatters directory. When the
formatters directory is updated with the results of the new build
process, the 'formatters/html/fonts' directory should be deleted as the
font file is now placed in the dist directory along with the JS and CSS.

For the Elixir-based tests to result in up-to-date build dist contents I
found I had to empty the _build directory.
@DavidOliver
Copy link
Contributor Author

Hopefully the Elixir tests will pass once the esbuild arch issue in the CI pipeline has been sorted; they're passing for me locally.

@josevalim
Copy link
Member

josevalim commented Sep 14, 2022

Amazing work!

I found that simply swapping out webpack for esbuild in karma.conf.js resulted in all tests continuing to pass.

As long as they are running, I think we are good. :)

Hopefully the Elixir tests will pass once the esbuild arch issue in the CI pipeline has been sorted; they're passing for me locally.

It seems CI is failing because we have to rebuild the assets. Can you please do it this time exceptionally as part of your PR? Then I can rebuild after merge. :) Thank you!

"karma-chai-plugins": "^0.9.0",
"karma-chrome-launcher": "^3.1.1",
"karma-esbuild": "^2.2.5",
"karma-firefox-launcher": "^2.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

We may no longer need this, looking at:

browsers: [process.env.TRAVIS ? 'Firefox' : 'Chrome'],

I assume TRAVIS is not set anywhere :)

@jonatanklosko
Copy link
Member

Beautiful!! 🐱

@DavidOliver
Copy link
Contributor Author

@josevalim, yes, the karma tests run and pass for me okay.

$ npm run test

> test
> npx karma start ./karma.conf.js --single-run

14 09 2022 15:27:07.428:INFO [esbuild]: Compiling to /tmp/ed9f0b8b17ec7fb2f8587559e7233a35-bundle.js...
14 09 2022 15:27:07.476:INFO [esbuild]: Compiling done (48ms)
14 09 2022 15:27:07.478:INFO [karma-server]: Karma v6.4.0 server started at http://localhost:9876/
14 09 2022 15:27:07.479:INFO [launcher]: Launching browsers Chrome with concurrency unlimited
14 09 2022 15:27:07.482:INFO [launcher]: Starting browser Chrome
14 09 2022 15:27:08.284:INFO [Chrome 105.0.0.0 (Linux x86_64)]: Connected on socket qEdPf8lxANdgnXLlAAAB with id 79475589
Chrome 105.0.0.0 (Linux x86_64): Executed 18 of 18 SUCCESS (0.001 secs / 0.006 secs)
TOTAL: 18 SUCCESS

I can add the built results in formatters, but I think that we'll still get the following EBADPLATFORM error here at Github's CI?

Run npm install --prefix assets
  npm install --prefix assets
  shell: /usr/bin/bash -e {0}
npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@[2](https://github.com/elixir-lang/ex_doc/actions/runs/3046512307/jobs/4909412480#step:4:2). I'll try to do my best with it!
npm ERR! code EBADPLATFORM
npm ERR! notsup Unsupported platform for esbuild-android-arm64@0.15.7: wanted {"os":"android","arch":"arm64"} (current: {"os":"linux","arch":"x64"})
npm ERR! notsup Valid OS:    android
npm ERR! notsup Valid Arch:  arm64
npm ERR! notsup Actual OS:   linux
npm ERR! notsup Actual Arch: x64

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2022-09-1[3](https://github.com/elixir-lang/ex_doc/actions/runs/3046512307/jobs/4909412480#step:4:3)T1[5](https://github.com/elixir-lang/ex_doc/actions/runs/3046512307/jobs/4909412480#step:4:6)_51_01_[9](https://github.com/elixir-lang/ex_doc/actions/runs/3046512307/jobs/4909412480#step:4:10)21Z-debug.log
Error: Process completed with exit code 1.

@DavidOliver
Copy link
Contributor Author

@josevalim, @jonatanklosko, should we also update to Node.js 16 LTS in the Github checks? It was released 2021-04-20 and is what I'm running locally with successful results.

Come to think of it, I wonder if that might be worth trying to see if it solves the CI esbuild installation issue?

@DavidOliver
Copy link
Contributor Author

@josevalim, I think using a newer version of Node.js may be the way to go: evanw/esbuild#1707

I'll add a commit updating ci.yml and see what happens.

@jonatanklosko
Copy link
Member

jonatanklosko commented Sep 14, 2022

Sounds like a good idea to me!

Hopefully this will allow for the esbuild package to be installed.

actions/checkout also updated to v3.

Step names are also removed for consistency.
@josevalim
Copy link
Member

@DavidOliver I think the Elixir suite will require the precompiled assets. :)

@DavidOliver
Copy link
Contributor Author

DavidOliver commented Sep 14, 2022

Thanks! :)

I think it's just the question of whether or not include Firefox in the JS tests left. Let me know either way and I'll simplify the karma config accordingly.

Edit: the remaining check seems to have hung on a post-run action, but the tests passed.

Edit 2: the check has finished after a good few minutes.

@DavidOliver
Copy link
Contributor Author

Should I also update the checkout action from v2 to v3 for the mix_test job?

@josevalim
Copy link
Member

I think it's just the question of whether or not include Firefox in the JS tests left. Let me know either way and I'll simplify the karma config accordingly.

It is your call :)

Should I also update the checkout action from v2 to v3 for the mix_test job?

Sure!

@DavidOliver
Copy link
Contributor Author

A few more commits pushed. Please check you're happy with the change to LICENSE.

I plan to add a development/watch task, but will come back to that another day.

@josevalim josevalim merged commit 7d400e5 into elixir-lang:main Sep 15, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim
Copy link
Member

MASSIVE effort, thank you!

@josevalim
Copy link
Member

And building it is so much faster!

@cw789
Copy link
Contributor

cw789 commented Sep 16, 2022

Thank you so much!

@cw789
Copy link
Contributor

cw789 commented Sep 16, 2022

Works also perfectly fine with latest Node version (18.9.0).

@DavidOliver
Copy link
Contributor Author

No problem. I don't yet have any Elixir libraries to document, but I'm glad to have been able to help simplify here. Thanks to you and everyone who provides tools and docs!

@cw789, great - good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants