Chore: add build step to bundle client-side assets #588

kaicataldo · 2019-07-01T01:57:58Z

I believe I have everything working except for the styling of the JS in the demo. orion/editor/built-editor.js relies on RequireJS to request orion/editor/stylers/application_javascript/syntax, and it's currently not able to find the module. I dug into this some more, and Orion requires the syntax highlighting file asynchronously at run time. I struggled with it for a bit and found it easier to just switch it out for CodeMirror (the Babel REPL uses this as well).

There are a lot of ways we can organize the code, and after exploring a few options this felt like the solution with the least amount of developer friction and overhead. It gets rid of some top level directories (I'd ideally like to move a lot of these top level directories into sub-directories to clean up the root directory of the repo) and puts all the client-side assets in one assets/ directory (which is what Jekyll suggests). I didn't rename styles to css, but we definitely can.

Happy to discuss other ways of doing things, though, if others have strong opinions!

Landing this opens us up a lot of options to us, and I wanted to keep this (mostly) limited to implementing a build step in a sane way.

Finally, some things we definitely need to do if we land this as-is:

We'll also need to update the release script before the next release. That being said, I think it could be nice to explore instead bundling latest Espree/ESLint in this repository at build time in the future.
We'll need to change the command Netlify runs before this is merged so that it'll build correctly (note that this is why the deployment preview is broken - neither the styles nor the JS are being built right now).

Note: The design is slightly different for the demo, but I think it's a little cleaner looking:

I removed the border because I think it's a little cleaner looking
I changed the error underline from a red squiggly line to a red dotted line because the squiggly line wouldn't show up correctly when highlighting one character (which we do for a syntax error)

See updated design below:

kaicataldo · 2019-07-01T01:59:38Z

.babelrc

+      ["@babel/preset-env", {
+        "modules": false,
+        "targets": {
+          "browsers": ["last 3 versions"]


What do we want to do for this? Relevant discussion: #543

If we don't want to think about this yet, we can also specify forceAllTransforms: true, and it will compile everything to be ES5-compatible.

kaicataldo · 2019-07-01T02:01:52Z

.eslintrc.js

+    root: true,
+    extends: ["eslint"],
+    parserOptions: {
+        ecmaVersion: 2019


I'd ideally like to start using ES2015+ features, but I do think it's important we stick to features available in Espree so that we can dogfood it in our site. Does limiting ourselves to features available in Espree (so, features that have reached stage 4 and are supported by the parser) sound like a good policy?

I think we should document this in the README (and maybe also create a CONTRIBUTING.md document)

kaicataldo · 2019-07-01T02:03:49Z

_config.dev.yml

@@ -15,6 +15,7 @@ exclude:
  - Gemfile.lock
  - package.json
  - node_modules
+  - src


Since it's not ignoring assets/build, this ensures Jekyll waits to regenerate the site until after Webpack finishes and outputs the bundles (I don't think this really matters what order they run in, but this prevents Jekyll from regenerating the site when src files are edited and then once more when the bundles are outputted).

_config.yml

kaicataldo · 2019-07-01T02:06:40Z

_tools/fetch-sponsors.js

@@ -13,7 +13,7 @@
 //-----------------------------------------------------------------------------

 const fs = require("fs");
-const fetch = require("node-fetch");
+const fetch = require("node-fetch"); // eslint-disable-line node/no-unpublished-require


I'm not sure why this is warning at the moment. Any ideas?

node-fetch is declared at devDependencies, so the eslint-plugin-node reported it.

That makes sense!

I know this isn't the rule's current behavior, but it seems like it shouldn't report when the root package.json has private: true?

@mysticatea Does that make sense? Maybe I'm missing something.

can we disable these rules in eslintrc? (since it is not a node lib.)

If you have files: [...] in your package.json, with publish files/dirs, that error should disappear.

@aladdin-add’s suggestion makes sense to me. Will disable for the website.

_tools/fetch-team-data.js

package.json

g-plane · 2019-07-01T02:13:16Z

webpack.config.js

+    },
+    output: {
+        path: path.join(__dirname, "assets/build/js"),
+        filename: "[name].js"


Do we need content hash here to avoid browsers cache issue?

That's a good question. Going off this blog post by Netlify, it seems like we shouldn't need to. This seems to be the same conclusion the Babel team came to.

kaicataldo · 2019-07-05T22:42:59Z

webpack.config.js

+                exclude: /node_modules|src\/js\/vendor/
+            },
+            {
+                test: /\.less$/,


Letting Webpack bundle these styles with their relevant JS bundles seems like an easier way to do this to me, now that the demo also requires the CodeMirror styles and less doesn't support running over multiple files. The alternative is to set up some kind script that runs less multiple times, but this felt like a cleaner solution to me. Thoughts?

kaicataldo · 2019-07-06T01:10:05Z

_config.yml

+exclude:
+  - node_modules
+  - src
+  - vendor


This fixes the Travis CI build

kaicataldo · 2019-07-06T01:56:02Z

Putting the do not merge label on this since the deploy preview version of the site isn't able to load all assets at the moment (I’m not seeing this locally so I’ll need to dig into it a bit to see what’s going on).

ilyavolodin · 2019-07-06T02:21:57Z

@kaicataldo Could you possibly separate CodeMirror into it's own PR that we can merge before we do this one? I would like to see those changes before we do a big-bang build changes.

kaicataldo · 2019-07-06T04:00:08Z

@ilyavolodin I do generally try to decouple tasks like this, but the reason I switched out Orion for CodeMirror is because it was difficult to get Orion to play nicely with webpack and ES modules.

I can spend time getting CodeMirror to work with the RequireJS setup we currently have (maybe by using a CDN?), but I’m not sure it buys us much if we’re going to be adding the bundling step soon anyway (unless you foresee us not merging this PR for a while).

Edit: For reference, it's not a huge change, but the bulk of the changes are here (I bundle the CodeMirror CSS from node_modules in the next commit).

kaicataldo · 2019-07-06T14:35:34Z

As a follow up to deploy preview version not being able to load assets - it's because the URLs are using {{ site.url }} and so the deploy preview environment is requesting assets from eslint.org. I don't think we have to fix deploy previews in this PR (and would rather decouple that from this task), but we could maybe use something like https://github.com/jayvdb/jekyll-netlify to access the deploy preview URL.

Edit: Fixed by #593

kaicataldo · 2019-07-10T04:58:22Z

Apologies that this is a large PR - a lot of it is the removal of files and package-lock.json changes. I think I did a pretty good job of isolating individual tasks in commits, and I think reviewing commit by commit makes it a lot easier to review.

kaicataldo · 2019-07-10T15:50:15Z

As discussed via email, I'm going to go ahead and merge this. Happy to continue discussing any of these decisions!

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jul 1, 2019

kaicataldo added chore This change is not user-facing do not merge This pull request should not be merged yet infrastructure Relates to the tools used to develop the website and removed triage An ESLint team member will look at this issue soon labels Jul 1, 2019

kaicataldo commented Jul 1, 2019

View reviewed changes

_config.yml Outdated Show resolved Hide resolved

kaicataldo commented Jul 1, 2019

View reviewed changes

_tools/fetch-team-data.js Outdated Show resolved Hide resolved

kaicataldo commented Jul 1, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

g-plane reviewed Jul 1, 2019

View reviewed changes

kaicataldo force-pushed the build-step branch from 37a1b49 to dc6c4eb Compare July 3, 2019 16:03

kaicataldo commented Jul 5, 2019

View reviewed changes

kaicataldo removed the do not merge This pull request should not be merged yet label Jul 5, 2019

kaicataldo changed the title ~~WIP: Add build step~~ Chore: Add build step Jul 5, 2019

kaicataldo changed the title ~~Chore: Add build step~~ Chore: add build step Jul 5, 2019

kaicataldo changed the title ~~Chore: add build step~~ Chore: add build step to bundle client-side assets Jul 5, 2019

kaicataldo commented Jul 6, 2019

View reviewed changes

_config.yml

exclude:

- node_modules

- src

- vendor

Copy link

Member Author

kaicataldo Jul 6, 2019

Choose a reason for hiding this comment

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

This fixes the Travis CI build

kaicataldo force-pushed the build-step branch 3 times, most recently from 9ec53aa to e5e3b08 Compare July 6, 2019 01:49

kaicataldo added the do not merge This pull request should not be merged yet label Jul 6, 2019

kaicataldo removed the do not merge This pull request should not be merged yet label Jul 6, 2019

kaicataldo force-pushed the build-step branch from 9139219 to e5e3b08 Compare July 6, 2019 15:01

kaicataldo added 9 commits July 10, 2019 04:33

Chore: add client-side assets build step

c2b7e43

Organize all assets in assets/ dir

d348be0

Switch Demo from Orion to CodeMirror

00386b9

Bundles parser.js and demo.js styles using Webpack

a9e37cc

Remove unused dependency

bb5bad7

Update .eslintignore

4289907

change class name for consistency

8a8b0a2

Fix Travis build

8d54bd5

Always clear text markers

3cfa9ab

kaicataldo force-pushed the build-step branch from d8e1218 to 3cfa9ab Compare July 10, 2019 04:34

Disable eslint-plugin-node rules for all JS files

fe028cf

kaicataldo merged commit fb130f8 into master Jul 10, 2019

kaicataldo deleted the build-step branch July 10, 2019 16:02

kaicataldo mentioned this pull request Jul 11, 2019

Chore: update path for release bundles eslint/eslint#11977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: add build step to bundle client-side assets #588

Chore: add build step to bundle client-side assets #588

kaicataldo commented Jul 1, 2019 •

edited

Loading

kaicataldo Jul 1, 2019

kaicataldo Jul 5, 2019 •

edited

Loading

kaicataldo Jul 1, 2019

kaicataldo Jul 5, 2019 •

edited

Loading

kaicataldo Jul 1, 2019 •

edited

Loading

kaicataldo Jul 1, 2019

g-plane Jul 1, 2019

kaicataldo Jul 1, 2019

kaicataldo Jul 6, 2019

kaicataldo Jul 10, 2019

aladdin-add Jul 10, 2019

mysticatea Jul 10, 2019

kaicataldo Jul 10, 2019

g-plane Jul 1, 2019

kaicataldo Jul 1, 2019

kaicataldo Jul 5, 2019 •

edited

Loading

kaicataldo Jul 6, 2019

kaicataldo commented Jul 6, 2019 •

edited

Loading

ilyavolodin commented Jul 6, 2019

kaicataldo commented Jul 6, 2019 •

edited

Loading

kaicataldo commented Jul 6, 2019 •

edited

Loading

kaicataldo commented Jul 10, 2019 •

edited

Loading

kaicataldo commented Jul 10, 2019

Chore: add build step to bundle client-side assets #588

Chore: add build step to bundle client-side assets #588

Conversation

kaicataldo commented Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

kaicataldo Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

kaicataldo Jul 1, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

kaicataldo Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Jul 6, 2019 • edited Loading

ilyavolodin commented Jul 6, 2019

kaicataldo commented Jul 6, 2019 • edited Loading

kaicataldo commented Jul 6, 2019 • edited Loading

kaicataldo commented Jul 10, 2019 • edited Loading

kaicataldo commented Jul 10, 2019

kaicataldo commented Jul 1, 2019 •

edited

Loading

kaicataldo Jul 5, 2019 •

edited

Loading

kaicataldo Jul 5, 2019 •

edited

Loading

kaicataldo Jul 1, 2019 •

edited

Loading

kaicataldo Jul 5, 2019 •

edited

Loading

kaicataldo commented Jul 6, 2019 •

edited

Loading

kaicataldo commented Jul 6, 2019 •

edited

Loading

kaicataldo commented Jul 6, 2019 •

edited

Loading

kaicataldo commented Jul 10, 2019 •

edited

Loading