Skip to content
This repository has been archived by the owner on Sep 2, 2020. It is now read-only.

Chore: replace Rollup.js bundler with Webpack #128

Merged
merged 4 commits into from
Feb 22, 2018
Merged

Conversation

niedzielski
Copy link
Contributor

  • This initial Webpack refactor patch aims for parity with the previous
    Rollup.js implementation. The configuration was largely derived from
    project Marvin. There are many enhancements possible with the new
    configuration but an effort to keep changes minimal where practical
    was made. For example, subsequent patches may diversify the library
    entry points offered, automatically vendor prefix all CSS, or inline
    image assets as data URIs.

    https://phabricator.wikimedia.org/source/marvin/browse/master/

  • The unusual Rollup.js Babel configuration is simplified and
    consolidated at the project root.

  • An empty PostCSS configuration is included for later auto-vendor
    prefixing.

  • The JavaScript and CSS artifacts have been minimized in production
    builds. Notably, the JavaScript build product is now half what it was.

  • A superfluous entry, build/wikimedia-page-library-override.js, is now
    generated and may safely be ignored. This is noted in the readme.

    Entry chunks for .css files also creating .js files webpack-contrib/extract-text-webpack-plugin#518

  • All demo URLs have been updated to reference webpack-dev-server
    outputs.

  • There are no anticipated client changes necessary but integrators
    should smoke test CSS and JavaScript functionality, especially on
    older devices.

  • The NPM clean script has been removed as this is part of the build
    process provided by Webpack and the "Clean for WebPack" plugin.

  • The build:watch NPM script is preserved to retain existing app
    developer workflows. App devs should verify that their processes are
    unaltered.

- This initial Webpack refactor patch aims for parity with the previous
  Rollup.js implementation. The configuration was largely derived from
  project Marvin. There are many enhancements possible with the new
  configuration but an effort to keep changes minimal where practical
  was made. For example, subsequent patches may diversify the library
  entry points offered, automatically vendor prefix all CSS, or inline
  image assets as data URIs.

  https://phabricator.wikimedia.org/source/marvin/browse/master/

- The unusual Rollup.js Babel configuration is simplified and
  consolidated at the project root.

- An empty PostCSS configuration is included for later auto-vendor
  prefixing.

- The JavaScript and CSS artifacts have been minimized in production
  builds. Notably, the JavaScript build product is now half what it was.

- A superfluous entry, build/wikimedia-page-library-override.js, is now
  generated and may safely be ignored. This is noted in the readme.

  webpack-contrib/extract-text-webpack-plugin#518

- All demo URLs have been updated to reference webpack-dev-server
  outputs.

- There are no anticipated client changes necessary but integrators
  should smoke test CSS and JavaScript functionality, especially on
  older devices.

- The NPM clean script has been removed as this is part of the build
  process provided by Webpack and the "Clean for WebPack" plugin.

- The build:watch NPM script is preserved to retain existing app
  developer workflows. App devs should verify that their processes are
  unaltered.
@montehurd
Copy link
Collaborator

Regarding replacing rollup.cssBundler.js, the order the CSS appeared in the final output file matters.

Do the CSS chunks still appear in the same order the files are referenced in index.js?

@niedzielski niedzielski changed the base branch from chore/test-size to master February 1, 2018 23:20
@niedzielski
Copy link
Contributor Author

@montehurd, I believe that concatenation order is based on import order (starting with the entry point, src/transform/index.js in our usage). In any case, I think the only spot this mattered was for the theme transform CSS and if you run npm run build:watch it will generate the un-minified CSS which clearly has the theme transform on top.

@niedzielski
Copy link
Contributor Author

@montehurd let me me know if you want to walk through these changes on Hangouts or if some other work is wanted 👍 I'm eager to get this in!

@montehurd
Copy link
Collaborator

@niedzielski oh a hangout would be great! sorry about the CR delay - got pulled into reading lists native bits for a while :)

readme.md Outdated
@@ -21,8 +21,11 @@ Presently we are consolidating duplicate Android and iOS Wikipedia app implement
### What wikimedia-page-library delivers
- **wikimedia-page-library-transform.js** bundle of all transform JS
- **wikimedia-page-library-transform.css** bundle of all CSS required by the bundled transform JS
- **wikimedia-page-library-transform.js** an unwanted [extraneous build product] that may safely be ignored
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename this guy.

The apps want to use their own minifiers for the time being. Remove the
minified build products until they're wanted.
@niedzielski
Copy link
Contributor Author

@montehurd feedback addressed and ready for review.

@montehurd
Copy link
Collaborator

montehurd commented Feb 20, 2018

@niedzielski

Heya quick question.

I gave this a quick integration tire-kicking this morning and had one issue... I made a change to a css file and JS file and ran "npm run-script build", but didn't see wikimedia-page-library-transform.css reflect the CSS change. I double-checked that switching back to master and running "npm run-script build" did cause the css file to be updated.

Is there a new building step I need to take when testing changes locally?

Opps needed to run npm update.

@niedzielski
Copy link
Contributor Author

@montehurd, this appears to work correctly for me locally:

$ npm run build
$ git add -f build
$ sed -ri s%#222%blue% src/transform/FooterReadMore.css
$ npm run build
$ git --no-pager diff build/wikimedia-page-library-transform.css
diff --git a/build/wikimedia-page-library-transform.css b/build/wikimedia-page-library-transform.css
index 16e9cab..0679c6c 100644
--- a/build/wikimedia-page-library-transform.css
+++ b/build/wikimedia-page-library-transform.css
@@ -967,7 +967,7 @@ html[dir='rtl'] .pagelib_footer_menu_item_subtitle {
 }
 
 .pagelib_theme_black .pagelib_footer_readmore_page {
-  background-color: #222;
+  background-color: blue;
   box-shadow: 0px 2px 2px rgba(34, 34, 34, 0.8);
 }
 
@@ -979,7 +979,7 @@ html[dir='rtl'] .pagelib_footer_menu_item_subtitle {
 }
 
 .pagelib_footer_readmore_page_title {
-  color: #222;
+  color: blue;
   display: block;
   overflow: hidden;
   text-overflow: ellipsis;

git pull, npm install, and try again?

Copy link
Collaborator

@montehurd montehurd left a comment

Choose a reason for hiding this comment

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

Been hammering this for a while and re-testing all transforms... so far seem solid! :)

@montehurd montehurd merged commit 534e8f1 into master Feb 22, 2018
@niedzielski niedzielski deleted the chore/webpack branch February 23, 2018 00:33
montehurd added a commit that referenced this pull request May 4, 2018
Noticed on `enwiki > Javascript`. The collapsed tables borders were gone when using sepia or dark themes.

Caused by using the '-p' webpack switch, which caused the CSS to be minified ( webpack-contrib/html-loader#65 (comment) ), which changed the order of CSS, which was important. Specifically, the `ThemeTransform` CSS needs to be 1st, as outlined here https://github.com/wikimedia/wikimedia-page-library/blob/master/src/transform/index.js#L3 and here #128 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants