Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Proof of concept: Add build step for demos #405

Closed
wants to merge 1 commit into from
Closed

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 5, 2017

@eslint/eslint-team

Hi! We've discussed (and haven't decided what to do about) adding a build step for the JS on our site. I decided to take a bit of time to see how hard it would be to implement, and here's the result.

Pros:

  • Smaller bundles (we can even split JS that is common between bundles into its own bundle so that it can be cached between pages) and faster load times (I know the demo as it currently stands takes a long time to initially render)
  • Compiling JSX ahead of time instead of at runtime like we do now
  • Code is easier to understand and to contribute to!
  • We get to start using npm and its ecosystem for package management, making it easier to update/change packages and improve things in the future (i.e. things like switching out the code editor package, rewriting in another framework, etc. all become much easier to implement)

Cons:

  • We have the added complexity of a build step

This PR contains the minimum amount of changes to get everything working. If we were to land this, I would want to follow up by updating the code with ES6+ syntax, changing the directory structure of the JS around a bit so it makes more sense (will require a change in the ESLint release script for the browserify function in Makefile.js), and take a look at what we can do to use npm to manage the final package in the js/vendor directory.

As far as actually running the build step goes, it seems to me like we have a few options:

  • Use a precommit Git hook to build any time a commit is created (What I opted for in this PR)
  • Have some kind of deployment/release process for eslint.github.io that will run the job automatically
  • Trigger a build somehow with ESLint's release script

I implemented the precommit Git hook in this PR because it gets it working now and doesn't preclude automating it some other way in the future.

The goal of this is to give us something concrete to discuss. Again, this is the bare minimum required to get this working, and I know I'd want to make a lot of other improvements should we decide to go this route.

@kaicataldo kaicataldo force-pushed the build-demos branch 2 times, most recently from 8a7ada7 to 1298d40 Compare September 5, 2017 02:48
@gyandeeps
Copy link
Member

gyandeeps commented Sep 5, 2017

@kaicataldo great work and I like the overall idea.

We need something for when the build script runs, can we

  • Use travis to do this? (every master commit builds the site and puts it inside gh-pages branch)
  • Use our bot to do the same thing?
  • Use jenkins to build the site? (run everyday and manually on need bases)

I really dont like precommits hooks because it mean more merge conflicts...

@kaicataldo
Copy link
Member Author

kaicataldo commented Sep 5, 2017

Ah, seeing one error that will need to be taken care of - it results in the code in the editor not being styled. Everything else should be working correctly.

screen shot 2017-09-04 at 10 57 09 pm

@kaicataldo
Copy link
Member Author

kaicataldo commented Sep 5, 2017

@gyandeeps Totally agree! Automating it would be best :) This was just a fast way to get it up and working. I like the idea of Travis doing it automatically!

@ilyavolodin
Copy link
Member

My personal opinion is that advantages that this brings does not overweight additional complexity. We only make occasional changes to the demo, so easier to contribute doesn't bring a huge amount of value. Easier to understand is, I think, a personal preference. For me, ES5 is easier to understand that ES6, since I don't work with it too much. Smaller bundles are overweight by mad complexity webpack config brings in with it. And usage of NPM packages is nice, but, again, we don't change demo nearly as often for this to make a big impact.
JSX pre-compilation is the only real advantage that I see. But that can really be done with a very simple build script.

Again, take everything above with a grain of salt. This is my personal opinion, and if everyone else feels like it's a good addition, I'm not going to stand in a way.

@kaicataldo
Copy link
Member Author

kaicataldo commented Sep 5, 2017

@ilyavolodin Fair points!

For me, ES5 is easier to understand that ES6, since I don't work with it too much.

Sorry, I should have been clearer about my "easier to understand" comment. Most React tutorials and examples I've come across (including the official docs) are written in ES6+, and from my experience with seeing Babel/ESLint issues, a lot of people who start with React are used to writing ES6+ (sometimes, perhaps, to a fault - it seems that a lot of people aren't aware of what the browser they're shipping code to can actually run).

Now, that doesn't mean that we have to go that route! Just clarifying.

JSX pre-compilation is the only real advantage that I see. But that can really be done with a very simple build script.

Mind explaining how this is any different from this proposed solution? The webpack config is pretty slim, and we could still run it manually with npm run build.

@ilyavolodin
Copy link
Member

Valid point on tutorials and examples for React. In terms of JSX pre-compile, it's not much different then what you are proposing, although for just JSX compilation, you could probably do that without bringing in the whole Webpack (webpack is like a nuclear option, it's so incredibly complex and poorly documented, that I always try to stay away from it until it's absolutely necessary). But if JSX compilation is the only advantage, I'm not sure it's worth it.

@not-an-aardvark
Copy link
Member

I'm 👍 for this provided that we don't have to commit built files into source control along with the regular code. I think we should have a e.g. a Travis task that builds stuff from master and pushes built files to gh-pages, then serve the site from that branch.

@kaicataldo
Copy link
Member Author

Wanted to check in and see if opinions have changed on this at all. I like @not-an-aardvark's suggestion a lot, and would definitely be up for doing some of the heavy lifting on this if we decide to go that route.

@mysticatea
Copy link
Member

Oh, I'm sorry, I had not realized this issue.

I had a plan to propose a similar thing since I have made Playground for eslint-plugin-vue and gotten some experience about such demo site.

In the playground, it imports eslint directly and does not need the browserify version of ESLint (src/app-state/eslint.js#L1, webpack.config.js#L106). So my idea was that I make eslint-demo package and ESLint release script uses the package instead of making the browserify version. The eslint-demo package includes the source code of the demo app, it's separated from this repo.

$ eslint-demo-generate lib/linter.js -o ../eslint.github.io/js/app/demo.js

(Additionally, we can use the demo app for offline and users are going to get a popup if a new version is available.)

@kaicataldo
Copy link
Member Author

kaicataldo commented Dec 12, 2017

I like that a lot! So it sounds like a few people like this idea and have proposed the following:

  • Automate the build step during releases
  • Create an eslint-demo package that can be consumed by browsers

@ilyavolodin Do the tasks above ease your concerns at all (particularly the first one)? As I mentioned, I still think this would allow us to iterate on the demo a lot faster as well as hopefully encourage more community contributions.

If @ilyavolodin is on board, how do we want to proceed with this? Should we make a long lived feature branch and push these changes to it as a basis for this work? I'm excited about this and, if everyone is on board, I'm happy to take on a lot of this work. Let me know and I can revisit this PR and clean things up.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Dec 12, 2017

Just to throw another idea out there: ES6 is now supported by all modern browsers, and import will also be supported in all modern browsers soon (based on this, it currently works everywhere except Firefox). If we're willing to drop support for IE11, we could use modern syntax without needing a build step.

@kaicataldo
Copy link
Member Author

@not-an-aardvark My personal take on that is that I'd still rather not be running the JSX transform in the browser, and I'd also rather not arbitrarily limit our browser support for our own convenience.

@ilyavolodin
Copy link
Member

As I mentioned before, if the rest of the team feels that the build step is helpful and needed, I have no problem adding it. My general approach is to avoid things that can be avoided, but if the benefits overweigh complexity - I'm fine with it.

@platinumazure
Copy link
Member

@kaicataldo Where are we at on this? Is this still something we want or need to do?

@kaicataldo
Copy link
Member Author

I'm not sure. Would this maybe worth discussing at the next TSC meeting? It seems like there's no one strongly opposed to this. I'm willing to work on this if it's clear we do want to do this!

@platinumazure
Copy link
Member

@kaicataldo Do we have a concrete proposal at this point? This is a PR but I also saw a few other ideas (e.g., from @not-an-aardvark and @mysticatea). If you're able to synthesize those into a TSC Summary/Question, I certainly think we could add it to the agenda. But I wouldn't want to take a lot of time to ideate on exactly how we would do this, personally.

@ilyavolodin
Copy link
Member

I'm not sure it's worth bringing this to TSC meeting. Two people who oppose build steps are me and @nzakas I generally highly dislike transpilers if they are not required, but as I said, I'm not going to stay in a way, if the rest of the team feels it provides advantage, and @nzakas will most likely not be available for TSC meeting, so having it on the agenda seems unnecessary.

@kaicataldo
Copy link
Member Author

I think my previous summary is still accurate: #405 (comment)

@eslint/eslint-team How does everyone feel about this?

@not-an-aardvark
Copy link
Member

#405 (comment) is still an accurate description of my view.

@hzoo
Copy link
Member

hzoo commented Jan 30, 2018

Babel's website has a build step for our repl (we moved it to react recently via babel/website#1297). We also moved to netlify.com from github pages and it allowed any build step with jekyll but now we are going to try out docusaurus.io for versioning (babel/website#1485).

@kaicataldo
Copy link
Member Author

kaicataldo commented Feb 6, 2018

@eslint/eslint-team Would moving to something like Netlify be adequate for automating the build process and having continuous deployment? Or would we want to continue using GitHub pages and push built assets to a gh-pages branch?

We could actually try Netlify out right now and point it to a branch with a build step and see if we like the workflow. This wouldn't affect the current site at all (we'd simultaneously have the test version of the site up on Netlify).

One nice plus is that in either case we'd have a lot more flexibility regarding what static site generation framework we use in the future.

@mysticatea Is #405 (comment) something you'd still like to do?

@kaicataldo
Copy link
Member Author

Discussed in the Gitter a bit after the above comment, and given that there is some opposition to this and the discussion has stalled, I'm going to close this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants