Exploration of different config + expose webpack config #222

CompuIves · 2016-11-07T14:53:28Z

We currently use package.json for our configuration. I think we should move away from using package.json and use something like next.config.js since it gives us more power than JSON.

An example is the issue for custom Webpack support: #40.
With a config like this you could do this for example:

// next.config.js

export default {
  webpack: (webpackConfig) => {
    const newConfig = { ...webpackConfig };
    newConfig.module.preloaders.push({ test: /\.js$/, loader: 'eslint-loader' });
    return newConfig;
  },
  cdn: false
}

Which is in my opinion better than creating a new file for webpack since this is more centralised. This does however still give users the option to use different files, for the webpack example:

// next.config.js

export default {
  webpack: require('./webpack').default,
  cdn: false
}

These are just my thoughts, I found out that I needed a sort of config file in .js when starting a PR for this project. Very curious what others think of this 😄.

frol · 2016-11-07T21:44:55Z

Disclaimer: I am not a JS developer, so I am not familiar with the traditions in JS world.

Config files in JS enable great flexibility, but at a cost of greater complexity and potential misuse and errors. In Python (which is my preferred language), it is common to use config files written in Python.

Overall, I like this idea.

frol · 2016-11-07T21:47:15Z

server/config.js

    } else {
      throw err
    }
  }

  // no try-cache, it must be a valid json
-  const config = JSON.parse(data).next || {}
+  const config = module.default || module || {}


FYI, the above comment is irrelevant for the updated implementation.

rauchg · 2016-11-07T23:30:20Z

This is a pretty great idea I think

Florian-R · 2016-11-08T09:36:52Z

If this one get merged (and I hope it will), should this fall back to the old behaviour, so this is not a breaking change?

CompuIves · 2016-11-08T18:34:56Z

@Florian-R You mean also read package.json for CND settings? We could do this, although it does add an extra bit of complexity since then we're reading two files for configuration.

frol · 2016-11-08T18:46:19Z

@Florian-R There existed only one config option: cdn=true. I don't think it is so much of a trouble to move one line from package.json to next.config.js.

Florian-R · 2016-11-09T09:38:51Z

@frol You're right I missed that, don't know why I was sure there was others options. This indeed shouldn't break any code, dismiss my previous comments.

CompuIves · 2016-11-10T12:56:15Z

This PR now also includes #174

giladgray · 2016-11-10T18:46:04Z

server/build/webpack.js

@@ -171,4 +172,7 @@ export default async function createCompiler (dir, { hotReload = false } = {}) {
      return interpolateNames.get(this.resourcePath) || url
    }
  })
+  const config = await getConfig(dir)


why is this async? i don't see any way for it to be async.

Oh right! That need was removed since we now use require. Good catch, I'll update the PR.

rauchg · 2016-11-16T18:42:48Z

Important: since this effectively deprecates package.next (which I'm 100% ok with), we should probably WARN if we find it

arunoda

@CompuIves could you check these suggestion.
After that, we could get ship this PR :)

arunoda · 2016-11-17T03:55:57Z

server/config.js


-  let data
+  let module
  try {


I think it's better to do a sync FS check rather doing this.
That's cleaner.

arunoda · 2016-11-17T05:15:41Z

server/config.js

-const defaultConfig = { cdn: true }
+const defaultConfig = {
+  cdn: true,
+  webpack: (cfg, hotReload) => cfg


It's a pretty good idea if we could log something, when we are using this user supplied webpack config decorator.
So, I suggest to not to define this.
And when we are using this, we can check for webpack decorator and log it.

arunoda · 2016-11-17T05:16:31Z

server/build/webpack.js

@@ -171,4 +172,7 @@ export default async function createCompiler (dir, { hotReload = false } = {}) {
      return interpolateNames.get(this.resourcePath) || url
    }
  })
+  const config = getConfig(dir)
+  const userWebpackConfig = config.webpack(webpackConfig, hotReload)


Do this only if user provides a webpack decorator.
If there's a one, we should log it so the user know this is happening.

arunoda · 2016-11-17T05:17:43Z

server/config.js

    } else {
      throw err
    }
  }

-  // no try-cache, it must be a valid json
-  const config = JSON.parse(data).next || {}
+  const config = module.default || module || {}


We we should also read the package.json and check for the next config. If it's there, we should get it. (Only the cdn field).
And if we found it, we need log a message as it's deprecated.

@nkzawa @rauchg do we have any specific format to log these messages.

no, I think

jonaswindey · 2016-11-17T08:41:57Z

Could we have a variable className on https://github.com/zeit/next.js/blob/master/lib/document.js#L11 in this config too?

That would cover #251

CompuIves · 2016-11-17T11:28:52Z

@arunoda Processed your feedback!
Maybe we could make a helper function in the future to show warnings/notices/errors with the use of chalk to make them stand out more.

@jonaswindey I think we should create a separate PR for that issue so we can put this PR live sooner.

rauchg · 2016-11-17T16:03:54Z

@jonaswindey @CompuIves the right way to address that is #251

rauchg · 2016-11-18T00:55:58Z

It just occurred to me that it might be smart to restart the next process (with the same flags) when this file changes? 🤔

rauchg · 2016-11-18T00:56:22Z

Unless, of course, it's tractable to reload everything without a full restart

arunoda

This is beautiful. We just need some few messages changes. I've added comments for them.

You may also need to rebase again since there are some conflicts.

Also we need to add a section to doc about configuring webpack config. Here's a suggestion for the layout:

Add sub section about Customizing webpack config.
Tell about the main goal of this.
Show a simple example (like importing images directly)
Then, ask to use this with care. (Since user could broke our default config)
So show safe operations like adding plugins, aliases, loaders etc.

Also try to keep the doc for this minimal and we can have a full detailed section on the Wiki.

arunoda · 2016-11-18T00:49:01Z

server/build/webpack.js

@@ -171,4 +172,10 @@ export default async function createCompiler (dir, { hotReload = false } = {}) {
      return interpolateNames.get(this.resourcePath) || url
    }
  })
+  const config = getConfig(dir)
+  if (config.webpack) {
+    console.log('Using Webpack config function defined in next.config.js.')


Add a "> " before the message. Like this:

console.log('> Using Webpack config function defined in next.config.js.')

arunoda · 2016-11-18T00:55:32Z

server/config.js

+  }
+
+  if (packageConfig) {
+    console.warn("WARNING: You're using package.json as source of config for next.js. Please use next.config.js instead.")


Change this to:

console.log("> [warn] You are using package.json for next.js config. Use next.config.js instead.");

Just FYI: Try not to use "Please" in front of messages. It's sounds like commanding.
I tend to use that and one of my UK friend asked to drop that. Messages sounds nice after that :)

I see, thanks for the tip!

CompuIves · 2016-11-19T22:21:58Z

@arunoda Added docs, let me know what you think! I'll rebase tomorrow, out of time now 😅

@rauchg Hmm, this is a valid point. I'll check if there is a way to restart the server when the config file changes.

arunoda · 2016-11-19T23:01:36Z

@rauchg That's a good point, but it seems like it's hard to do that via webpack itself.

We have to do a hard process restart, may be we could do it using the grunt config easily.

arunoda · 2016-11-19T23:02:29Z

@CompuIves yep. Changes are great. Shall we try @rauchg's suggestion too.

CompuIves · 2016-11-20T14:24:50Z

@arunoda Added config watching, it kills the old child process and start a new one when it sees a change in next.config.js (only for dev mode).

Also rebased with master!

nkzawa · 2016-11-20T14:47:47Z

I wonder if we should:

transpile the next.config.js (support import export, async/await etc)
support asynchronous operations in each config

export default {
  async webpack (webpackConfig, hotReload) {
    const newConfig = { ...webpackConfig }
    newConfig.something = await getSomething()
    return newConfig
  }
}

coveralls · 2016-12-17T16:38:10Z

Coverage increased (+2.9%) to 60.036% when pulling 2316e55 on CompuIves:new_config into b62a0e8 on zeit:master.

CompuIves · 2016-12-17T16:38:35Z

Rebased master and did the required changes.

I stumbled upon the item of CDN in the FAQ of the README. Do we still need this?

arunoda · 2016-12-17T17:21:08Z

README.md

+```javascript
+// next.config.js
+module.exports = {
+  cdn: true


We don't have a cdn option. May be we need to re-phrase this.

arunoda · 2016-12-17T17:21:55Z

README.md

@@ -423,7 +483,7 @@ For this reason we want to promote a situation where users can share the cache f

 We are committed to providing a great uptime and levels of security for our CDN. Even so, we also **automatically fall back** if the CDN script fails to load [with a simple trick](http://www.hanselman.com/blog/CDNsFailButYourScriptsDontHaveToFallbackFromCDNToLocalJQuery.aspx).

-To turn the CDN off, just set `{ “next”: { “cdn”: false } }` in `package.json`.
+To turn the CDN off, just set `module.exports = { cdn: false }` in `next.config.js`.


Don't worry about this one. We'll remove it anyway.

arunoda · 2016-12-17T17:23:49Z

@CompuIves I added some comments.

rauchg · 2016-12-17T18:38:19Z

I'll remove the cdn notes myself

rauchg · 2016-12-17T18:38:23Z

Thanks @CompuIves

arunoda · 2016-12-17T18:38:36Z

Awesome!

CompuIves · 2016-12-17T18:48:51Z

Nice! Was great to help! 🎉

CompuIves mentioned this pull request Nov 7, 2016

expose webpack config #174

Closed

frol reviewed Nov 7, 2016

View reviewed changes

CompuIves changed the title ~~Exploration of different config~~ Exploration of different config + expose webpack config Nov 10, 2016

giladgray reviewed Nov 10, 2016

View reviewed changes

dbo mentioned this pull request Nov 16, 2016

Debugging next.js apps #260

Closed

arunoda self-assigned this Nov 16, 2016

This was referenced Nov 16, 2016

Bind operator: unexpected token #266

Closed

Issues with libraries #119

Closed

arunoda suggested changes Nov 17, 2016

View reviewed changes

arunoda approved these changes Nov 18, 2016

View reviewed changes

arunoda suggested changes Nov 18, 2016

View reviewed changes

CompuIves force-pushed the new_config branch from 097febe to 6c7eaa9 Compare November 20, 2016 14:10

Ives van Hoorne added 12 commits December 17, 2016 17:27

Include hotReload option

c612a6d

Remove async/await for getConfig

2f90055

Read package.json, show warning when webpack in config is defined

20f847b

Prepend warning message with WARNING

eb850c0

Update log statements

e142729

Documentation

bb5d635

Restart server on change of config

99d0fb3

Fix process handling and cases where there is no config

5615086

Also restart server when config file gets deleted

f3c3bb4

Changed second parameter of webpack to config

c23c5b2

Support for returning Promise

0d77b3f

Update documentation, fix bug with webpack config

4c8ab59

CompuIves force-pushed the new_config branch from d6fe625 to 4c8ab59 Compare December 17, 2016 16:31

Remove package.json, cdn and hotReload from config

2316e55

arunoda reviewed Dec 17, 2016

View reviewed changes

arunoda approved these changes Dec 17, 2016

View reviewed changes

arunoda mentioned this pull request Dec 17, 2016

Add X-Powered-By header. #416

Merged

rauchg merged commit 5ab7463 into vercel:master Dec 17, 2016

lukeed mentioned this pull request Dec 27, 2016

Cannot alias React #532

Closed

souporserious mentioned this pull request Dec 29, 2016

Allow override configs kitze/custom-react-scripts#46

Closed

jagreehal mentioned this pull request Jan 1, 2017

Adding Typescript loader in next.config.js #607

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Exploration of different config + expose webpack config #222

Exploration of different config + expose webpack config #222

Conversation

CompuIves commented Nov 7, 2016 • edited Loading

frol commented Nov 7, 2016

Choose a reason for hiding this comment

rauchg commented Nov 7, 2016

Florian-R commented Nov 8, 2016

CompuIves commented Nov 8, 2016

frol commented Nov 8, 2016

Florian-R commented Nov 9, 2016

CompuIves commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauchg commented Nov 16, 2016

arunoda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonaswindey commented Nov 17, 2016

CompuIves commented Nov 17, 2016

rauchg commented Nov 17, 2016

rauchg commented Nov 18, 2016

rauchg commented Nov 18, 2016

arunoda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CompuIves commented Nov 19, 2016 • edited Loading

arunoda commented Nov 19, 2016

arunoda commented Nov 19, 2016 • edited Loading

CompuIves commented Nov 20, 2016 • edited Loading

nkzawa commented Nov 20, 2016 • edited Loading

coveralls commented Dec 17, 2016 • edited Loading

CompuIves commented Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunoda commented Dec 17, 2016

rauchg commented Dec 17, 2016

rauchg commented Dec 17, 2016

arunoda commented Dec 17, 2016

CompuIves commented Dec 17, 2016

CompuIves commented Nov 7, 2016 •

edited

Loading

CompuIves commented Nov 19, 2016 •

edited

Loading

arunoda commented Nov 19, 2016 •

edited

Loading

CompuIves commented Nov 20, 2016 •

edited

Loading

nkzawa commented Nov 20, 2016 •

edited

Loading

coveralls commented Dec 17, 2016 •

edited

Loading

CompuIves commented Dec 17, 2016 •

edited

Loading