test: Setup dev server for rendering JS and Sass #2

lynnmercier · 2018-03-07T01:55:09Z

Allows each new package to setup it's own page in the dev server (aka http://localhost:8080/test/screenshot/foo-package/). Developers just need to update test/screenshot/webpack.config.js with their input Sass and JS, and output CSS and JS. Then they reference that output CSS and JS in an index.html

moog16 · 2018-03-07T04:03:22Z

I think having an index.html file for each component screenshot test will be a lot of unnecessary duplicate work. I think if we utilize React it'll greatly simplify adding/removing screenshot tests. Yes we will have only 1 bundle.css and bundle.js file, but the end user (the team) all have fast connections and computers. We should really be driving for faster/easier development.

Also the screenshot tests lend themselves to using a SPA framework like React, similar to the demo pages. But we can chat about this more tomorrow :)

kfranqueiro · 2018-03-07T15:32:35Z

I would suspect that the decision for separate bundles per test is VERY intentional; IIRC @acdvorak intends to do similar for our screenshot tests in the main repo. (Actually, I'm at least a little bit confused here... we ARE still doing screenshot tests in the main repo, right? IMO it's more important for them to exist there than in the React wrapper, because all of the styles being tested are the main repo's responsibility...)

Look at how long it takes the dev server in the main repo to initially start up and compile CSS, then talk to me again about fast computers. (It takes a full minute for me, which is frankly unacceptable, and I look forward to the day we alleviate that with better-organized bundles for screenshot test pages.)

kfranqueiro · 2018-03-07T15:34:43Z

test/screenshot/index.html

@@ -0,0 +1,3 @@
+<html>
+  <a href="temporary-package">Temporary Package</a>
+</html>


You're missing newlines at end of files FYI

moog16 · 2018-03-07T17:17:08Z

Well the start up for webpack does take a while, but the load time in a browser of a large bundle.js(~1MB) vs a small bundle.js(~100kB) served locally I would argue is not perceivable. If it was served across the wire to more people, then yes it would be a problem. But I think the use case is, download the repo and run the instance locally. The webpack bundling time is another issue altogether.

What I was saying is having an html file per screenshot test doesn't make sense to me. The purpose of React is to minimize the amount of scaffolding and setup you have. I suspect that in the Catalog pages we wouldn't have an individual html file per component, so I don't think we should here. The piece of info I'm missing and cannot speak to is how the screenshot tests are setup.

kfranqueiro · 2018-03-07T18:54:30Z

Webpack rebuild time is especially important for developer experience, i.e. not having to wait 30 seconds just to check a change during development. Browser load time also matters for screenshot tests, since I think we're already wary of how long even our unit tests in the core repo take, and IIUC CrossBrowserTesting is not a local network use case.

Meanwhile, I'm pretty sure I do remember us talking about intentionally having an html file per catalog page... I realize that's an atypical application of React, but I got the impression that using React for the catalog was more of (a) a templating solution and (b) a gateway to showcasing MDC React in the catalog, rather than an intentional direction toward making the catalog a SPA. The catalog's pages should load as quickly as possible. That all is tangential to the organization of this repo though.

…ce code

lynnmercier · 2018-03-07T21:47:45Z

I updated this PR so it actually uses React (and added some new lines).

The start up time of this dev server is still almost 6 seconds, mostly because of babel-ifying react-dom.

The reload time is fine right now....but theres not a lot of code. And after some investigations we cant really figure out when webpack decides to recompile stuff /shrug.

Matt is going to work on a separate PR which does the exact same thing as this PR....except it uses React Router instead of creating new index.html for each "page" in our dev server.

We'll compare and contrast PRs once that is ready.

acdvorak · 2018-03-08T00:50:01Z

test/screenshot/temporary-package/index.html

@@ -0,0 +1,9 @@
+<html>


Add <!DOCTYPE html> before this line.

Without it, all browsers will go into "quirks mode" instead of "standards mode", and things will render differently.

(The HTML5 spec also says doctype is required.)

acdvorak · 2018-03-08T00:51:03Z

test/screenshot/temporary-package/index.html

+   <script src="bundle.js" async></script>
+ </head>
+ <body>
+ 	<div id="app"></div>


How did a tab character \t sneak in here? 😛

acdvorak · 2018-03-08T00:51:22Z

test/screenshot/temporary-package/index.html

@@ -0,0 +1,9 @@
+<html>
+ <head>


Re-indent this file (everything inside <html> is missing 1 leading space).

acdvorak · 2018-03-08T00:52:02Z

test/screenshot/webpack.config.js

+const {sassBundle, javaScriptBundle} = require('./webpack-bundles');
+
+module.exports = [
+sassBundle('./test/screenshot/temporary-package/temporary-package.scss', 'temporary-package/bundle.css'),


Indentation

acdvorak · 2018-03-08T00:52:06Z

test/screenshot/webpack.config.js

+
+module.exports = [
+sassBundle('./test/screenshot/temporary-package/temporary-package.scss', 'temporary-package/bundle.css'),
+javaScriptBundle('./test/screenshot/temporary-package/index.js', 'temporary-package/bundle.js')


acdvorak · 2018-03-08T03:46:10Z

My two cents:

We probably don't need a templating language for screenshot test pages.
I don't see any reason to use React Router for screenshot tests. The test pages don't need any UI or navigation at all. They're test pages, not apps.
Test files should be as simple as humanly possible. Abstractions in tests lead to bugs.
We definitely want separate CSS/JS bundles for every component's screenshot test page, or a single bundle with multiple modules (one module per screenshot test page). This ensures fast recompilation.
The React repo shouldn't need to compile very much Sass (if any), except maybe for theming.

1. Templating language

Unlike the Catalog site, React screenshot tests probably don't need any of the core features that templating systems provide: Reusable UI elements, data binding, and interactivity. If that's the case, there's no reason to use templates for test pages.

The only exception I can think of would be for components that require a LOT of markup, like mdc-text-field or mdc-select. Using templates could make those components easier to write, but we'd also run the risk of making the tests harder to understand and debug.

For our first iteration, I would much prefer to err on the side of simplicity.

If, after we've written a bunch of screenshot tests, it becomes clear that I was totally wrong and templates are indeed necessary, we can always add them.

YAGNI until proven otherwise 😀

2. Static hosting and deep links

We need to be able to:

Host the screenshot tests on a static site (Amazon S3, Firebase Hosting, etc.) without running our own Node server
- This enables us to deploy and view the test pages anywhere - even a local filesystem with no HTTP server
- Makes it easier to share WIP demos with designers
- Much cheaper than running our own server
- Simpler and less likely to break
Link directly to individual screenshot test pages (e.g., https://mdc-web-demo.firebaseapp.com/pr/123/test/screenshot/button/index.html)

What's more, test pages should be dead simple: No navigation, no UI (except for the components being tested of course). So why do we need React Router?

3. Simple test pages

Screenshot test pages should have nearly zero non-baseline HTML/CSS/JS (except for custom theme mixins).

For example:

button-test.html

<link rel="stylesheet" href="node_modules/@material/button/dist/mdc.button.css">
<link rel="stylesheet" href="button-test.css">
<script src="node_modules/material-components-web/dist/material-components-web.js"></script>
<script src="button-test.js"></script>

<div>
  <button class="mdc-button">Flat</button>
  <button class="mdc-button mdc-button--raised">Raised</button>
  <button class="mdc-button mdc-button--stroked">Stroked</button>
</div>

<div>
  <button class="mdc-button test-button">Flat</button>
  <button class="mdc-button mdc-button--raised test-button--raised">Raised</button>
  <button class="mdc-button mdc-button--stroked test-button--stroked">Stroked</button>
</div>

button-test.scss

@import "@material/button/mixins";

.test-button {
  @include mdc-button-ink-color(red);
}
.test-button--raised {
  @include mdc-button-filled-accessible(red);
}
.test-button--stroked {
  @include mdc-button-ink-color(red);
  @include mdc-button-stroke-color(red);
}

button-test.js

mdc.autoInit(); // or whatever

4. Webpack bundles/modules

Decoupled Webpack bundles/modules === much faster compilation.

I learned this lesson the hard way with MDC Web's demo pages 🤪

Sass compilation is by far the slowest part of our build process today. It takes ~15-30 seconds to recompile the demo server styles. In contrast, material-components-web.js recompiles almost instantly.

It turns out that when you have a single, monolithic Webpack module (as we previously did for the demo site), changing even a single line of Sass requires recompiling ALL the Sass - every single time.

That's not a problem if you have a small amount of Sass, but as the codebase grows organically, you inevitably start to see the build and recompile times creep steadily upward, incrementally and imperceptibly, until suddenly it takes 20 seconds to recompile anything, because you have to recompile everything.

5. Sass for React

React doesn't need to compile any baseline MDC code at all. It can just use the precompiled .css and .js files from MDC's public npm modules.

We also probably don't need any React-specific Sass (except maybe for theme customization?).

moog16 · 2018-03-08T17:24:35Z

test/screenshot/webpack-bundles.js

@@ -0,0 +1,63 @@
+module.exports.sassBundle = function(sassInput, cssOutput) {


I think cutting this down from sassBundle and javaScript bundle into just 1 xyzBundle method will cut down the compile time. When you pass a new config into the array in multicompiler mode, webpack thinks it is a new bundle. Yes we want the css file to be separate, but I think by passing the name prop to the the file-loader will create a separate css file.

The only drawback(up for debate) to having 1 bundle is that you must import sass files into the JS files. But I think this improves build times.

moog16 · 2018-03-08T17:26:53Z

package.json

@@ -3,6 +3,7 @@
  "description": "Material Components React",
  "license": "Apache-2.0",
  "scripts": {
+    "start": "webpack-dev-server --config test/screenshot/webpack.config.js --content-base test/screenshot",


I don't think we can do the --content-base option here. I think we need the webpack.config.js file to live in the root. I don't think it transform the files in the packages dir. Try importing a React Component into the screenshot tests, and I don't think you'll get JSX errors.

moog16

Left a few comments about things I learned yesterday. I have another branch with the changes mentioned. Maybe we can get this PR merged in and my branch can be a post PR?

test: Setup dev server for rendering JS and Sass

cb1e8cf

kfranqueiro reviewed Mar 7, 2018

View reviewed changes

lynnmercier added 2 commits March 7, 2018 10:19

serve files from localhost root, not test/screenshot

139d9ce

update dev server to render using React

0da8d1a

lynnmercier added 3 commits March 7, 2018 11:53

temp commit to show a second package

adc1ed8

remove the second package, and update test to actually call some sour…

ab1433c

…ce code

add some new lines

3ef5d6c

acdvorak reviewed Mar 8, 2018

View reviewed changes

moog16 reviewed Mar 8, 2018

View reviewed changes

lynnmercier added 4 commits March 8, 2018 17:21

working with optimize-css-assets plugin to remove duplicate CSS

53be3a8

linting my react

1c0c516

adding some new lines

d47cf2a

fixing indents

d7e529c

moog16 approved these changes Mar 9, 2018

View reviewed changes

lynnmercier merged commit 8264c48 into master Mar 9, 2018

lynnmercier deleted the test/dev-server branch March 9, 2018 01:37

moog16 mentioned this pull request Dec 14, 2018

feat(top-app-bar): is missing dense variant #518

Closed

moog16 mentioned this pull request Apr 5, 2019

<Dialog /> is not a controlled component #772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Setup dev server for rendering JS and Sass #2

test: Setup dev server for rendering JS and Sass #2

lynnmercier commented Mar 7, 2018

moog16 commented Mar 7, 2018

kfranqueiro commented Mar 7, 2018 •

edited

Loading

kfranqueiro Mar 7, 2018

moog16 commented Mar 7, 2018

kfranqueiro commented Mar 7, 2018

lynnmercier commented Mar 7, 2018

acdvorak Mar 8, 2018

acdvorak Mar 8, 2018

acdvorak Mar 8, 2018

acdvorak Mar 8, 2018

acdvorak Mar 8, 2018

acdvorak commented Mar 8, 2018 •

edited

Loading

moog16 Mar 8, 2018

moog16 Mar 8, 2018

lynnmercier Mar 8, 2018

moog16 left a comment

		@@ -0,0 +1,63 @@
		module.exports.sassBundle = function(sassInput, cssOutput) {

test: Setup dev server for rendering JS and Sass #2

test: Setup dev server for rendering JS and Sass #2

Conversation

lynnmercier commented Mar 7, 2018

moog16 commented Mar 7, 2018

kfranqueiro commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

moog16 commented Mar 7, 2018

kfranqueiro commented Mar 7, 2018

lynnmercier commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdvorak commented Mar 8, 2018 • edited Loading

1. Templating language

2. Static hosting and deep links

3. Simple test pages

4. Webpack bundles/modules

5. Sass for React

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moog16 left a comment

Choose a reason for hiding this comment

kfranqueiro commented Mar 7, 2018 •

edited

Loading

acdvorak commented Mar 8, 2018 •

edited

Loading