Skip to content

Conversation

gj
Copy link
Contributor

@gj gj commented Sep 17, 2018

Closes #873

  • Removes Gulp
  • Upgrades Webpack, Babel, Mocha, Karma, ESLint, Less, Lodash, and more to the latest version of each

Regarding @josevalim's comment, the coupling between Gulp, Webpack, and Karma made it a bit awkward to update Webpack in isolation.

This PR is all about feature parity, with one exception: there's no more equivalent of the npm run clean Gulp task that removed build artifacts. I set Webpack up to automatically remove build artifacts before executing a production build, which retains most of the same functionality as before. If a script solution is preferably to manual rimraffing, I can add a little script for the npm run clean functionality.

If this PR is accepted, I would like to simplify the test setup in a followup PR. Right now we're loading a full browser environment but then only running unit tests, which seems like a bit of overkill to me.

npm audit

  • Before: found 84 vulnerabilities (36 low, 24 moderate, 23 high, 1 critical)
  • After: found 2 low severity vulnerabilities

gj added 20 commits September 17, 2018 14:18
For some reason, the CommonJS-ready language files in the lib/ directory
are not present in the 9.12.0 release. That's what necessitates the
current convoluted build process (invoking tools/build.js and whatnot).
Since there's not much [difference](https://github.com/highlightjs/highlight.js/blob/master/CHANGES.md#version-9120)
between 9.11.0 and 9.12.0, it seems simpler to downgrade the dependency.
And remove same functionality from Gulp pipeline
browserslist config taken from autoprefixer config in Gulpfile
@sourcelevel-bot
Copy link

Hello, @gj! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@josevalim
Copy link
Member

Thanks @gj, this looks amazing! Three last questions :

  1. What about the generated .js size? Is it smaller now?

  2. I saw that we changed the approach to highlight.js but I am assuming that the end result is the same?

  3. Can we move the .eslint files at root to inside the assets directory?

❤️

@gj
Copy link
Contributor Author

gj commented Sep 21, 2018

@josevalim

  1. Before: EPUB CSS 5.45 kB; EPUB JS 34.18 kB; HTML CSS 23.12 kB; HTML JS 218.05 kB
    After: EPUB CSS 5.53 kB; EPUB JS 41.1 kB; HTML CSS 24.9 kB; HTML JS 169 kB
  2. Yep, the HTML docs get highlighted just as before. There's no code highlighting in EPUB files before this PR and still no code highlighting in EPUB files after this PR. I plan to take a look at that after this PR goes in.
  3. Yep, good idea. Moved everything asset-related into the assets/ dir except for package.json and package-lock.json (so that commands can still be run from the project root).

ETA: updated sizes after removing Elixir syntax highlighting import

EPUB code highlighting doesn't work before or after this PR, but that's
a bit outside the scope of this PR and should be addressed in a
follow-up
@josevalim
Copy link
Member

Thanks @gj, it looks good to go. Just one last change: can you please revert decc9e7 and rebuild the assets? The Elixir syntax highlightning is not done with JS, it is done while we generate the docs using the "makeup" tool. It only supports Elixir right now but the plan is to move to it little by little. :)

@gj
Copy link
Contributor Author

gj commented Sep 21, 2018

@josevalim haha I just noticed that too (in b44eba1)

Reverted and the CI is running 😄

@sourcelevel-bot
Copy link

Ebert has finished reviewing this Pull Request and has found:

  • 11 possible new issues (including those that may have been commented here).
  • 8 fixed issues! 🎉

You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/901.

@josevalim josevalim merged commit 347580f into elixir-lang:master Sep 21, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@gj gj deleted the update-js-build branch September 21, 2018 21:27
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.

2 participants