New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ES6 modular code via Babel and Webpack #375

Merged
merged 9 commits into from Feb 13, 2017

Conversation

Projects
None yet
3 participants
@Haroenv
Copy link
Member

Haroenv commented Feb 8, 2017

the tooling now includes yarn itself (which can be relied on, since netlify has yarn).

closes #330

The structure of the JS files now is (in my opinion) more reasonable to think about, and also will allow for faster development for further features.

jQuery, bootstrap are still loaded via cdn, because that's better for large bundles like those.

instantsearch, docsearch, moment are loaded via npm and bundled in (which saved approx. 300kb on the search page)

@Haroenv Haroenv force-pushed the algolia:frontend-tooling branch from f1753f5 to 6434c06 Feb 8, 2017

@Haroenv Haroenv changed the title configure webpack and babel Allow ES6 modular code via Babel and Webpack Feb 8, 2017

@vvo vvo force-pushed the algolia:frontend-tooling branch from 181489a to e984eb3 Feb 8, 2017

chore(tooling): Add webpack and babel to allow ES6 code
The JavaScript code of the website itself starts to be bigger than in
the beginings. To allow for a greater dev experience (write modern
JavaScript) and plan next iterations on the search and package detail
pages (use React), this PR propose to add Babel and Webpack to the
toolbet of the website.

The structure of the JS files now is (in my opinion) more reasonable to think about,
and also will allow for faster development for further features.

@vvo vvo force-pushed the algolia:frontend-tooling branch from e984eb3 to 26de192 Feb 8, 2017

@Daniel15
Copy link
Member

Daniel15 left a comment

Do we really need Webpack? Is it not sufficient to only use Babel?


test-jekyll:
ifndef JEKYLL_EXISTS
$(error Jekyll is not installed. Run `make install`)

This comment has been minimized.

@Daniel15

Daniel15 Feb 8, 2017

Member

Why remove this? The site still depends on Jekyll so the script should probably check that it exists.

This comment has been minimized.

@Haroenv

Haroenv Feb 8, 2017

Author Member

Because jekyll is executed via bundler, not via the jekyll standalone binary

@@ -1,8 +1,7 @@
---
layout: page
scripts:
- https://cdn.jsdelivr.net/docsearch.js/1/docsearch.min.js
- /js/docsearch.js
- "/js/build/documentation.js"

This comment has been minimized.

@Daniel15

Daniel15 Feb 8, 2017

Member

Would be good for these file paths to have a cache breaker... Can we do that? You can use assets-webpack-plugin to write the cache breakers into a JSON file if you use a filename like [name].js?[hash], could the site read that JSON file?

This comment has been minimized.

@Haroenv

Haroenv Feb 8, 2017

Author Member

I wanted to do that, but didn’t figure out how it’s done yet, your proposal sounds good.

This comment has been minimized.

@Haroenv

Haroenv Feb 9, 2017

Author Member

This is done now @Daniel15

@@ -0,0 +1,30 @@
export function handleTabs() {

This comment has been minimized.

@Daniel15

Daniel15 Feb 8, 2017

Member

What's the advantage of putting this in a separate file, given it's tightly-coupled to the page? I'd suggest just putting this in js/src/install.js directly

This comment has been minimized.

@Haroenv

Haroenv Feb 9, 2017

Author Member

I did this because I imagine that pages other than the install page could benefit from having the handling of tabs into the history

documentation: './js/src/documentation.js',
install: './js/src/install.js',
nightly: './js/src/nightly.js',
search: './js/src/search.js',

This comment has been minimized.

@Daniel15

Daniel15 Feb 8, 2017

Member

If you want to use Webpack, I think this should be done using code splitting... With this setup, some code required by two of the files (eg. if both install.js and nightly.js use the same module) will be duplicated across both bundles. Code splitting can pull the common code into a separate module that both can use.

This comment has been minimized.

@Haroenv

Haroenv Feb 8, 2017

Author Member

totally right, will be fixed!

This comment has been minimized.

@Haroenv

Haroenv Feb 9, 2017

Author Member

Hey @Daniel15, thanks for the suggestion. I've never done code splitting before, so now I manually did what code would be split too, with a hand-made common.js that hosts things that all pages have. Could you tell me how that would be set up properly? Thanks!

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented Feb 8, 2017

I like this idea 😃 I wonder if we could just use Babel for now, to reduce complexity? We don't really need any Webpack features yet.

@Haroenv

This comment has been minimized.

Copy link
Member Author

Haroenv commented Feb 8, 2017

Thanks for your review @Daniel15, It’s evening now for me, so I’ll answer quickly for now.

I’d prefer to have webpack in here, because then I can easily depend on let’s say React or something else to make the detail pages for the search. 🎉

@vvo

This comment has been minimized.

Copy link
Contributor

vvo commented Feb 8, 2017

(I worked a bit with @Haroenv on this)

Using only babel may work I guess even if we want to use React, we would have to inject React via script tags then (because Babel won't be able to inline what's inside node_modules right?).

It would also mean that we would not be able for now to use any dependency from npm, this may be a bit tricky at some point to have to rely only on script tags. As you want!

Haroenv added some commits Feb 9, 2017

fix non-display of initial results on Safari
fixes #4

Safari is more strict about closing brackets in querySelectors than Chrome, which just closes it for you
@Haroenv

This comment has been minimized.

Copy link
Member Author

Haroenv commented Feb 10, 2017

I fixed some small issues that creeped in, but now it should be good to merge I think 😄

@Daniel15

This comment has been minimized.

Copy link
Member

Daniel15 commented Feb 13, 2017

This looks pretty good to me. Tested a few pages on the Netlify preview (https://deploy-preview-375--yarnpkg.netlify.com/) and it all seems to be working.

The only thing I don't like is that there's two separate "build" commands: The build command in the make file (make build-production), and the build command in package.json (yarn run build), with no clear documentation on why this is the case. To me it seems like you could completely avoid the build script in package.json and just call node_modules/.bin/webpack in the Makefile. I'm going to merge this as-is, but that's an idea for cleaup if you submit future diffs. Thanks for your contribution!

@Daniel15 Daniel15 merged commit e5a02df into yarnpkg:master Feb 13, 2017

1 check passed

deploy/netlify Deploy preview ready!
Details

@Haroenv Haroenv deleted the algolia:frontend-tooling branch Apr 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment