Skip to content
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

Extract the Hapi Server to expose it for testing #277

Closed
wants to merge 3 commits into from

Conversation

benstepp
Copy link
Contributor

@benstepp benstepp commented May 7, 2016

  • The Hapi Server has been extracted so that it can be
    tested using the built in #inject() method.
  • Test support has been moved to /helpers. This is the default location
    that is ignored by ava.
  • Added a test to ensure that the user can override the default html.js
    in both the develop and build stages.

After some research I think using Hapi's inject is going to provide the best testing experience with the least amount of test setup. This way we have simpler test infrastructure that is less likely to be the cause of test failures.

The only code change was that the server is now started in lib/utils/develop and constructed in lib/utils/dev-server

* The Hapi Server has been extracted so that it can be
tested using the built in #inject() method.
* Test support has been moved to /helpers. This is the default location
that is ignored by `ava`.
* Added a test to ensure that the user can override the default html.js
in both the develop and build stages.

After some research I think using Hapi's inject is going to provide the
best testing experience. The only code change was that the server is now
started in `lib/utils/develop` and constructed in `lib/utils/dev-server`
@benstepp
Copy link
Contributor Author

benstepp commented May 7, 2016

I'll mess with babel tomorrow, I just wanted some feedback from CI. Maybe the require.resolve trick in the webpack config will take care of it.

Moves the htmlCompilerConfig to webpack.config where it belongs in
webpack.config.js

Drops babel-plugin-transform-object-rest-spread because we already use
babel-stage-0 in our .babelrc and this plugin is included in it. It
doesn't make sense to call it out twice.

Switched the `develop` stage babel-loader config to use stage-0 over
stage-1 because that is what we have specified in our package.json
This change makes it so the user doesn't need to specify their plugins.
If they want to override it they would need to modify the js loader
config using the modifyWebpackConfig API. Ideally we would support a
user provided .babelrc as well.

```
exports.modifyWebpackConfig = function(config, env) {
  config.removeLader('js')
  config.loader('js', {
    test: /\.jsx?$/,
    exclude: /node_modules/,
    loader: 'babel',
    query: {
      presets: [
        'babel-preset-react',
        'babel-preset-es2015',
        'babel-preset-stage-0',
      ].map(require.resolve),
      plugins: [
        'babel-plugin-add-module-exports',
        'babel-plugin-transform-decorators-legacy',
      ].map(require.resolve),
    },
  })
  return config;
}
```

Fixes gatsbyjs#235. Requires less user package.json dependencies unless the user
wants to add babel plugins (Users no longer need to provide
babel-plugins of any kind in their project as gatsby provides them)
@benstepp
Copy link
Contributor Author

benstepp commented May 7, 2016

@KyleAMathews I want to get some feedback on this approach. Basically we would be able to remove all of the babel stuff from starters because they don't need anything beyond what gatsby provides by default.

Rather than relying on babel's plugin resolving, we just pass resolved plugin paths to the babel-loader. A user could still override them with the modifyWebpackConfig API.

  • Easier on testing (no need to inject the webpack config into the hapi server)
  • Easier on users (no need to add a babelrc or babel-plugins to package.json)
  • Still allows for advanced babel config by overriding the loader

@KyleAMathews
Copy link
Contributor

@benstepp potentially I really like this. I really don't like that users have to supply the Babel config/plugins right now. The only question I have here is how does this work (as in #235) when Gatsby is in a sub-directory of a Babel-driven JS module. Say someone building a React component wants to use Gatsby for their documentation site so inside Gatsby they're requiring up a directory into the src of the React component e.g. (require('../../src/MyComponent')). And say they're using a custom .babelrc. Do you know if Babel would still use their Babel setup with this change?

@benstepp
Copy link
Contributor Author

benstepp commented May 8, 2016

tldr; babel takes anything for a plugin. we are just telling it explicitly rather than letting babel figure it out for us.


So the custom .babelrc is something I would like to support before this gets merged, as that would cause this to be a nonbreaking change. Basically this uses require.resolve to convert the babel-plugins into absolute paths relative to the gatsby installation. I ran into this idea when looking at a few issues to have the gatsby develop server use the gatsby's root level babel-plugins:

I went and looked at what babel allows for a plugin, and you can use pretty much anything (makes sense if maybe your company has private plugins for something). And it resolves them here:

The idea for supporting a custom .babelrc would be to:

  1. check if the user has one
  2. read the babelrc and JSON.parse it
  3. map all of the plugins into absolute paths (so if you require 5 directories up the babel stuff still works because babel will treat them as absolute-path plugins and not auto resolve them in the directory that was crawled up to). As long as the .babelrc in the root of the gatsby project can cover whatever components are above, everything will work fine.
  4. pass to webpack config.

If the user has a .babelrc they would have to also have any plugins they want to use specified locally (either in the gatsby directory or crawled up towards root. If we use require.resolve it will find them.) The user could copy their main top level .babelrc into the docs directory if they wanted.

We would have to change how react-hmre gets added as well to extend the user's babelrc rather than overwrite it. (probably with a plugins.push(require.resolve('babel-plugin-react-hmre'))) such that the user's babelrc is still relevant.

I have a strong feeling that this will also fix #129 by allowing the user to use whatever babel plugins they want. I'll explore some more tomorrow.

@benstepp
Copy link
Contributor Author

benstepp commented May 8, 2016

Closing in favor of #279. There was a lot of noise here and I would rather do that separately from the babel changes.

@benstepp benstepp closed this May 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants