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

Improve mode/NODE_ENV defaults and interaction #972

Merged
merged 1 commit into from Jul 13, 2018

Conversation

@edmorley
Copy link
Member

edmorley commented Jun 29, 2018

Previously the only way to override mode was by passing --mode on the command line. However this is not possible with all tools, since some (such as karma) reject unrecognised arguments.

Now:

  • mode is derived from NODE_ENV if --mode wasn't passed, and !production NODE_ENV falls back to mode development.
  • if --mode is passed, it takes priority over NODE_ENV.
  • if neither mode nor NODE_ENV is defined, then NODE_ENVis set toproduction, however mode` is left undefined, which causes webpack to output a helpful warning about relying on defaults.
  • the template test runner configs set a default NODE_ENV of test.
  • @neutrinojs/stylelint now also correctly sets failOnError.

Fixes #900.
Fixes #971.
Closes #955.

@edmorley edmorley added this to the v9 milestone Jun 29, 2018
@edmorley edmorley self-assigned this Jun 29, 2018
@edmorley edmorley changed the title Improve mode and NODE_ENV defaults and interaction Improve mode/NODE_ENV defaults and interaction Jun 29, 2018
@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Jun 29, 2018

Once we agree on the behaviour/UX I'll fix/add some tests :-)

@@ -1,3 +1,5 @@
const neutrino = require('./packages/neutrino');

process.env.NODE_ENV = process.env.NODE_ENV || 'lint';

This comment has been minimized.

Copy link
@edmorley

edmorley Jun 29, 2018

Author Member

Possible values instead of "lint" might be "test" or "production". Though given we use "test" for karma.config.js etc, it seems wrong to just call this "production".

ESLint config conditionals that choose to rely on NODE_ENV (eg enabling/disabling the no-console rule) should be using NODE_ENV !== 'development' rather than NODE_ENV === 'production' for compatibility with NODE_ENV='test' anyway -- so using NODE_ENV='lint' doesn't seem like it would cause any more problems.

This comment has been minimized.

Copy link
@edmorley

edmorley Jul 4, 2018

Author Member

@eliperelman thoughts about this part?

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 5, 2018

Member

I'm OK with this, although my initial gut reaction wishes this existed in eslint() instead of being dotfile boilerplate.

This comment has been minimized.

Copy link
@edmorley

edmorley Jul 5, 2018

Author Member

Yeah me too -- though I guess that would mean we would have to change the whole way the middlewares are instantiated.

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 5, 2018

Member

Yeah which is too much right now. The thing that prevents us from doing this is not being able to reliably infer when the NODE_ENV was set by Neutrino vs. the environment.

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 5, 2018

Member

I know this sounds crazy, but...

What if the Neutrino instance had options stating whether or not mode and NODE_ENV were specified, and the middleware could control the values?

// packages/neutrino/index.js
const neutrino = new Neutrino({
  mode: yargs.argv.mode,
  NODE_ENV: process.env.NODE_ENV
})

// packages/web/index.js
if (!neutrino.options.NODE_ENV) {
  process.env.NODE_ENV = 'production';
}

// packages/karma/index.js
neutrino.register('karma', (neutrino, override) => config => {
  if (!neutrino.options.NODE_ENV) {
    process.env.NODE_ENV = 'test';
  }
});

Obviously the above isn't correct, but conceptual. Thoughts?

This comment has been minimized.

Copy link
@edmorley

edmorley Jul 5, 2018

Author Member

That definitely gets us closer, however if I follow correctly, it would mean that the web preset (and any others) see the NODE_ENV as 'production' and not 'test', thereby meaning the parts of the config they contribute to the karma config, are using the wrong NODE_ENV.

Therefore it seems that the only way to solve this is make the meat of the presets be a step that occurs after they register themselves.

ie:

// registers the presets and what output commands they include,
// but doesn't actually generate any config or adjust NODE_ENV
neutrino.use(some_lint_preset);
neutrino.use(some_build_preset);
neutrino.use(some_test_preset);

// The output function then sets NODE_ENV if undefined (in this case to `"test"`)
// and then actually generates the config, where all presets get to see `NODE_ENV=test`
neutrino.karma();

This comment has been minimized.

Copy link
@edmorley

edmorley Jul 5, 2018

Author Member

(but that's a major change and would mean backwards incompatibility with all existing presets)

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 10, 2018

Member

I think keeping middleware compat is more important here.

// Development mode is most appropriate for a !production NODE_ENV (such as `NODE_ENV=test`).
mode = (process.env.NODE_ENV === 'production') ? 'production' : 'development';
} else {
throw new Error('Either --mode must be specified via the CLI, or else NODE_ENV be defined.');

This comment has been minimized.

Copy link
@edmorley

edmorley Jun 29, 2018

Author Member

Alternatives to throwing are:

  • default to NODE_ENV='production' and mode='production'
  • default to NODE_ENV='production' and don't set mode at all. This would mean for workflows that include a webpack compile (ie not yarn lint), webpack would output the "no mode was specified, using production" warning message, which might help solve the "prevent people from accidentally omitting mode/NODE_ENV` concern.

Both of the alternatives would mean we could drop the process.env.NODE_ENV = ... boilerplate from the template .eslintrc.js files -- though we would still need it for karma.config.js.

I'm on the fence as to which of these three options is best.

This comment has been minimized.

Copy link
@edmorley

edmorley Jul 4, 2018

Author Member

@eliperelman and this?

@@ -6,6 +6,8 @@ module.exports = (neutrino, { pluginId = 'stylelint', ...opts } = {}) => {
configBasedir: neutrino.options.root,
files: '**/*.+(css|scss|sass|less)',
context: neutrino.options.source,
// Fail for all of 'production', 'lint' and 'test'.
failOnError: process.env.NODE_ENV !== 'development',

This comment has been minimized.

Copy link
@edmorley

edmorley Jun 29, 2018

Author Member

This fixes #971.

@@ -84,7 +84,8 @@ module.exports = (neutrino, opts = {}) => {
include: !opts.include ? [neutrino.options.source, neutrino.options.tests] : undefined,
eslint: {
cache: true,
failOnError: neutrino.config.get('mode') === 'production',
// Fail for all of 'production', 'lint' and 'test'.
failOnError: process.env.NODE_ENV !== 'development',

This comment has been minimized.

Copy link
@edmorley

edmorley Jun 29, 2018

Author Member

I've flipped this, so it is set to true for yarn test too.

@edmorley edmorley requested a review from eliperelman Jun 29, 2018
@edmorley edmorley force-pushed the edmorley:mode-NODE_ENV-defaults branch from 1312d93 to 39ea238 Jun 29, 2018
@edmorley edmorley added bug and removed breaking change labels Jul 1, 2018
@edmorley edmorley force-pushed the edmorley:mode-NODE_ENV-defaults branch from 39ea238 to 1a8f663 Jul 1, 2018

if (!process.env.NODE_ENV) {
if (mode) {
process.env.NODE_ENV = mode;

This comment has been minimized.

Copy link
@eliperelman

eliperelman Jul 2, 2018

Member

Here we force NODE_ENV to always be the same value as --mode, but that's not what we want right? What about NODE_ENV=development webpack --mode production?

This comment has been minimized.

Copy link
@edmorley

edmorley Jul 2, 2018

Author Member

This was intentional (there's a note in the PR description), for the reasons under scenario G here:
#955 (comment)

...though open to counter-arguments :-)

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Jul 3, 2018

@eliperelman, any more thoughts on the UX choices? I'm keen to get them finalised, so I can updated the tests in this PR, then press on with the overall docs update in another PR.

(Context: I really want to get Treeherder builds faster soon (ie next week or two), which means a Neutrino 9 alpha by then, or else stopping using Neutrino and just switching to plain webpack)

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Jul 3, 2018

I still a little uneasy about #972 (comment) but I can't articulate my issue with it. I'd say let's roll with it, test it out, and revise if necessary.

@edmorley edmorley dismissed eliperelman’s stale review Jul 4, 2018

(Cancelling review until tests are updated)

Previously the only way to override `mode` was by passing `--mode` on
the command line. However this is not possible with all tools, since
some (such as karma) reject unrecognised arguments.

Now:
* `mode` is derived from `NODE_ENV` if `--mode` wasn't passed, and
  !production `NODE_ENV` falls back to mode `development`.
* if `--mode` is passed, it takes priority over `NODE_ENV`.
* if neither `mode` nor `NODE_ENV is defined, then `NODE_ENV` is set
  to `production`, however `mode` is left undefined, which causes
  webpack to output a helpful warning about relying on defaults.
* the template test runner configs set a default `NODE_ENV` of `test`.
* `@neutrinojs/stylelint` now also correctly sets `failOnError`.

Fixes #900.
Fixes #971.
Closes #955.
@edmorley edmorley force-pushed the edmorley:mode-NODE_ENV-defaults branch from 1a8f663 to 5855cba Jul 13, 2018
@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Jul 13, 2018

I've updated the PR - changes are:

  • if neither NODE_ENV nor mode is defined, now instead of throwing, NODE_ENV is set to production and mode left as undefined. This avoids having to set a default NODE_ENV in .eslintrc.js, and leaving mode as undefined means that when webpack is used, the webpack "mode wasn't set, falling back to defaults" message is shown, which should help educate users.
  • the tests have been updated / additional coverage added.
Copy link
Member

eliperelman left a comment

This looks good. I'm willing to roll with it and see how it does.

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Jul 13, 2018

👍

@edmorley edmorley merged commit 2dce8d2 into neutrinojs:master Jul 13, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@edmorley edmorley deleted the edmorley:mode-NODE_ENV-defaults branch Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.