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

Support building for a server-side target #59 #76

Merged
merged 1 commit into from
Aug 30, 2016
Merged

Support building for a server-side target #59 #76

merged 1 commit into from
Aug 30, 2016

Conversation

zamiang
Copy link
Contributor

@zamiang zamiang commented Jun 3, 2016

After our discussion in #59, I did a little digging and found what was wrong (I think?)

Unlike in your example app which is building for browsers, I am using webpack to build for a node target. Since there was no node option in the package.json, it would fall back to index.js and error trying to load lib/contentful.

This PR adds a node option to the package.json to support server-side builds such as this.

@Khaledgarbaya
Copy link
Contributor

Hi,

I think the problem here is your webpack configuration.

Webpack will load modules from the node_modules folder and bundle them in. While this is fine for frontend code, backend modules aren't prepared for this.

It would be nice if you share your webpack configuration or try to add this to your webpack.config.js

var fs = require('fs');

var nodeModules = {};
fs.readdirSync('node_modules')
  .filter(function(x) {
    return ['.bin'].indexOf(x) === -1;
  })
  .forEach(function(mod) {
    nodeModules[mod] = 'commonjs ' + mod;
  });

ps: I just tried contentfuljs in a small express app using webpack and I had no problem

Please let us know if this helped.
Best,
Khaled

@zamiang
Copy link
Contributor Author

zamiang commented Jun 4, 2016

Hey @Khaledgarbaya

Here is a webpack config where it fails for me. I believe the key here is the target: "node" declaration but perhaps may have something to do with babel-loader.

var commonLoaders = [
  {
    /*
     * TC39 categories proposals for babel in 4 stages
     * Read more http://babeljs.io/docs/usage/experimental/
     */
    test: /\.js$|\.jsx$/,
    loader: 'babel-loader',
    include: path.join(__dirname, '..', 'app'),
    exclude: path.join(__dirname, '..', 'node_modules')
  },
  { test: /\.json$/, loader: "json-loader" },
  { test: /\.css$/,
    loader: ExtractTextPlugin.extract('style-loader', 'css-loader?module!postcss-loader')
  }
];

module.exports = {
    name: "server-side rendering",
    context: path.join(__dirname, "..", "app"),
    entry: {
      server: "./server"
    },
    target: "node",
    output: {
      path: assetsPath,
      filename: "server.js",
      publicPath: publicPath,
      libraryTarget: "commonjs2"
    },
    module: {
      loaders: commonLoaders
    },
    resolve: {
      root: [path.join(__dirname, '..', 'app')],
      extensions: ['', '.js', '.jsx', '.css']
    },
    plugins: [
      new webpack.optimize.OccurenceOrderPlugin(),
      new ExtractTextPlugin("styles/main.css"),
      new webpack.optimize.UglifyJsPlugin({
        compressor: {
          warnings: false
        }
      }),
      new webpack.DefinePlugin({
        __DEVCLIENT__: false,
        __DEVSERVER__: false
      }),
      new InlineEnviromentVariablesPlugin({ NODE_ENV: 'production' })
    ],
    postcss: postCSSConfig
  }

@Khaledgarbaya
Copy link
Contributor

Hi,

I think it is related to the modules loading in my config I am using externals
So I have this.

var nodeModules = fs.readdirSync('node_modules')
  .filter(function(x) {
    return ['.bin'].indexOf(x) === -1;
  });
//......
var backendConfig = config({
  entry: [
    './src/main.js'
  ],
  target: 'node',
  output: {
    path: path.join(__dirname, 'build'),
    filename: 'backend.js'
  },
  node: {
    __dirname: true,
    __filename: true
  },
  externals: [
    function(context, request, callback) {
      var pathStart = request.split('/')[0];
      if (nodeModules.indexOf(pathStart) >= 0) {
        return callback(null, "commonjs " + request);
      };
      callback();
    }
  ],
//...... rest of the config

Can you try it and tell me if it is working.
Best,
Khaled

@zamiang
Copy link
Contributor Author

zamiang commented Jun 4, 2016

Interesting. That does fix the issue for me.

I really don't think adding that code is an acceptable solution for me. I would rather not introduce sophisticated logic into externals -- it is intended for things like externals: { jquery: "jQuery" }. Hopefully this test provides some insight into the root of the problem leading to a more sustainable fix.

FWIW, matching the node target to an appropriate file in package.json seems like a more appropriate solution. I haven't (knowingly!) encountered a library where the default main js file in package.json is not intended for primary use. Given that webpack supports many targets beyond 'node' and 'browser', maybe there are other patterns for setting up files for various targets that are more appropriate.

@Khaledgarbaya
Copy link
Contributor

Thanks for all the feedback, I will definitely look into that more with the team.
Have a great weekend :)

@trodrigues
Copy link
Contributor

Hello @zamiang

I was having a better look at this (I was away for the last week so couldn't look into this immediately), as I setup the current build, and I was investigating this a bit better and doing a few more tests.

Your solution does indeed seem to fix the issue and from what I can tell so far, it looks like the only thing we could really do from our side that could help out with people using targets in webpack other than browser.

However, this is one of those package.json fields (like browser) which are somehow "non standard", so what I'd like to understand is what it means and exactly and where it comes from, as I can't find any source of documentation for this, either on the webpack docs or anywhere else.

What I'm trying to understand is: is the node field in package.json a webpack thing, or is it maybe just a coincidence that this works? Also, if we'd add this, what impact could this possibly have on other build tools and/or JS environments?

I'll do some more investigation on this, but if you could provide a source of information for where this field comes from, that would be very helpful, as otherwise I'd feel a bit weary of adding something when we can't truly understand the full impact it would have.

@zamiang
Copy link
Contributor Author

zamiang commented Aug 21, 2016

Quick update, found some additional info about webpack targets. They are documented here: https://webpack.github.io/docs/configuration.html#target

I am still in a scenario where I do need to use the node target functionality and am unable to use the default web target.

Alternatively, if this is not a change you are interested in, I would love some guidance on how I might use a fork the project. Today's attempts have not been successful due to changes that happen when the module is packaged and published to npm vs how files appear in the repo.

@Khaledgarbaya
Copy link
Contributor

Hi @zamiang ,
Sorry I was away for a little bit. Let me take a look and get back to you asap.
Best,
Khaled

@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Changes Unknown when pulling 787f718 on zamiang:server-side-webpack into * on contentful:master*.

@Khaledgarbaya
Copy link
Contributor

Hi @zamiang ,
I will merge the pull request as it turns to be the only solution to your issue and I will keep an eye on the lib hopefully there is no side effects. Thanks for the contribution
Best,
Khaled

@Khaledgarbaya Khaledgarbaya merged commit c8bf9ac into contentful:master Aug 30, 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.

4 participants