Consider support for local requires in server.js

[matthewdavidson]

Just ran into this ...

Packaging components...an error happened while packaging /path/to/my/component: ./path/to/local/module.json not found. Only json files are require-able.

I get the design decision of limiting local requires to .json files - keeping server.js small and limiting it's responsibility to component data fetching / plumbing 👍 .

However, the existence of a hard limitation in the framework seems a little too restrictive IMHO:

• A component can still be "small" and still have local requires
• Component authors can circumvent this restriction anyway by using a module bundler to generate server.js (in doing so bundle all of their local requires into one file)
• There isn't much difference between npm dependencies and local dependencies yet one is allowed and the other isn't.

My particular use case is as follows (React server side rendering):

'use strict';

var React = require('react');
var ReactDOMServer = require('react-dom/server');

var Component = require('./path/to/my/component');

module.exports.data = function(context, callback) {
  var element = React.createElement(Component);
  var html = ReactDOMServer.renderToString(element);

  callback(null, { html: html });
};

Are there any major objections to this? Potential downsides I see are CLI bloat and percieved lack of control over server.js complexity. CLI bloat because we'd probably need some sort of module bundler to allow local requires. That being said we've discussed similar stuff in #339. And in terms of lack of control .... surely thats what code reviews are for 😇 .

[matteofigus]

There were a couple of reasons for doing this a while ago but I agree that's not an issue anymore.

We use webpack here for doing this - I would vote 👍 for putting this into the CLI and leaving to webpack to produce the bundled server.js. I think this would be the easiest way to achieve this as we wouldn't change anything on the publishing and execution, only thing would be to add the extra magic to the CLI.

[matteofigus]

Ah, there are other options apart from webpack obviously, we may do a spike to see what's the best way to do this :)

[matthewdavidson]

Yeah I would have suggested webpack too!

[matteofigus]

Also, @antwhite worked on this a while ago. I think we just may need to bring this code into the cli and add some testing: https://github.com/antwhite/oc-webpack

[nickbalestra]

@matthewdavidson @matteofigus I like the idea of relying on a bundler + transpiler to solve this and #339
As per the goals I would suggest to go with rollup/bublé instead of the webpack/babel option:

• rollup bundles are always smaller then browserify or webpack
• rollup config is way simpler then webpack
• bublé limits itself to ES features that can be compiled to compact, performant ES5 (plus JSX)
• bublé has no plugins or presets – less extensibility, but also zero configuration
• bublé alters code only where necessary (formatting and code style remain intact)
• bublé It's comparatively tiny and much faster then Babel

[matthewdavidson]

@matteofigus I've had a look at @antwhite's example - it's exactly the approach I mentioned above:

Component authors can circumvent this restriction anyway by using a module bundler to generate server.js (in doing so bundle all of their local requires into one file)

I'd like to have a go at a PR for a webpack solution if thats ok?

@nickbalestra, I'd heard of rollup before due to its tree-shaking ability but as far as I am aware it's not widely adopted or at least it's not as ubiquitous as webpack. Webpack 2 has tree shaking and will be stable very soon. I've never heard of bublé - I thought babel was the de-facto standard for compilation 🤔 . Either way i'd err on the side of webpack/babel just because I would feel confident in their future development / support / communities.

[matthewdavidson]

@nickbalestra also relevant since you mention it in your comment on #341.

create-react-app uses webpack & babel under the hood. Seeing as it's facebooks officially supported way of getting started with react i'd say they are a safe bet 😇 - although I do appreciate that this particular issue isn't related to front end framework choices in oc components themselves.

[matteofigus]

Very good points here. I think I would probably like to see both and then make a decision. Still, don't want to waste anyone's time for working on something that then gets thrown away.

My 2 cents, for me most important things are (for both bundler - webpack vs rollup - and transpiler - babel vs buble')

1. Community support: bundlers seem both really strong, in terms of transpilers babel seems a safer choice
2. Dimension: this stuff is going to be part of the CLI and would like to get the smaller amounts of modules there as sub-dependencies (we could spike both just on npm install + measuring the impact)
3. Secure: we are not integrating snyk yet but will soon. Whatever option we take I would like a completely green build when running a snyk test. Serverless architectures need to take security very seriously and my vision is to invest more on that soon.
4. Extensibility: when new ES6 features will come up, or will want to start using ES7, how easy will be to upgrade? I am mostly thinking about async/await here as JS community is in ❤️ with that and I would like to support it asap if that makes it to ES7 and node 7 supports it.

[nickbalestra]

Good points!
I'm fine with either. if we want to simply solve the initially mentioned goals with no string attached:

1. Local requires
2. es2015

then imho rollup/bublé just do the job.

But If we believe we need/want more powers then I agree that webpack/babel is the way. @matteofigus as per your points 1&4 I would say webpack/babel (both are more mature/battle-tested and have a huge community/plugins/you-name-it.

@matthewdavidson I'll be working on it this week and I would love your code-reviews / input / feedback as we move forward, so I guess we could work together on this.

About the webpack/oc dev integration I guess we want to have a webpack watcher -> so that any new webpack build triggers the oc dev watcher. Any other thoughts in this direction related to dx and what we would like/should have?

[matteofigus]

@nickbalestra in relation to the watcher I think it's almost as the opposite. OC has its own watcher during dev, once there is a change there is a "build" process that currently (re)generates the _package folder (which is also the publish output). The code for the server.js is mostly here:
https://github.com/opentable/oc/blob/master/src/cli/domain/package-server-script.js#L87-L98

I think we need to get rid of the require stuff (webpack takes care of it) and then add the babelification too. We can pair on this tomorrow ;)

[nickbalestra]

Yup that's it. Let's do it :)

[matthewdavidson]

@nickbalestra Awesome, count me in for reviews etc.

[nickbalestra]

@matteofigus, @matthewdavidson
I've added a branch -> https://github.com/opentable/oc/tree/package-server-webpack so that we can collaborate on pulling this off as discussed.

Tests are obviously broken at the moment, and the webpack config/build is far from production-grade. I've also removed for the moment the various things we were doing 'manually' like checking dependencies as I believe we should be able to get those from webpack.

Please let me know what you think about this very early concept.

• package-server-script
• bundle module

[matteofigus]

Done with #346