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

Add checks for middleware being used twice unintentionally #1162

Merged
merged 3 commits into from Oct 12, 2018

Conversation

@edmorley
Copy link
Member

edmorley commented Oct 10, 2018

To try and prevent the confusion caused when users do things like:

@edmorley edmorley added this to the v9 milestone Oct 10, 2018
@edmorley edmorley self-assigned this Oct 10, 2018
@edmorley edmorley requested a review from eliperelman Oct 10, 2018
@timkelty

This comment has been minimized.

Copy link
Contributor

timkelty commented Oct 11, 2018

using @neutrinojs/react followed by @neutrinojs/style, which causes incorrect extract and hot configuration:

@edmorley there is some crossover here with #964.

I've had a few people asking how they can configure @neutrinojs/style to have separate rules (e.g. one less and one sass), each with different test and loaders.

…and you can't. My only suggestion to them thus far was doing exactly what you're referencing here: use @neutrinojs/style after @neutrinojs/web|react|vue and configure a second separate rule.

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Oct 11, 2018

My only suggestion to them thus far was doing exactly what you're referencing here: use @neutrinojs/style after @neutrinojs/web|react|vue and configure a second separate rule.

The check in the PR uses neutrino.config.module.rules.has(options.ruleId), so if I've understood the workaround for #964 correctly, it shouldn't be affected, since people are using a different ruleId (otherwise the rules would have been overwritten anyway).

However perhaps the wording of the error message shown needs to say:
"... Or if using twice is intentional, remember to set a different ruleId" etc?
(I just thought it was implied given the first sentence of the message, but perhaps best to be explicit?)

@edmorley edmorley requested a review from timkelty Oct 11, 2018
packages/compile-loader/index.js Outdated Show resolved Hide resolved
packages/compile-loader/index.js Outdated Show resolved Hide resolved
@timkelty

This comment has been minimized.

Copy link
Contributor

timkelty commented Oct 11, 2018

@edmorley My only thought is you could consider simplifying the messages and including a link to a page in the docs detailing more specifics – but short of that, looks good!

@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Oct 11, 2018

Yeah true. I think the reason I didn't initially, is that there isn't a good permalink for many of the settings, since they appear in the middle of "here are all the options" sections. A hardcoded URL would also get out of date once we release new major versions of Neutrino (though we have this problem already in a few places).

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Oct 11, 2018

Another way we could have a more generic error message would be to subclass Error:

class DuplicateRuleError extends Error {
  constructor(name, ruleId) {
    super(
      `${name} has been used twice with the same `ruleId` of ${ruleId}.\n` +
      'If you are including this preset manually to customise the rules\n' +
      "configured by another preset, instead use that preset's own options to do so."
    );
  }
}

// ...

if (neutrino.config.module.rules.has(ruleId)) {
  throw new DuplicateRuleError('@neutrinojs/compile-loader', 'compile');
@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Oct 11, 2018

Yeah that may be better (I'd decided against it before since each message was subtly different eg the preset option name, but perhaps that's unnecessary). Should DuplicateRuleError live in neutrino?

@eliperelman

This comment has been minimized.

Copy link
Member

eliperelman commented Oct 11, 2018

Should DuplicateRuleError live in neutrino?

That would be my recommendation since every middleware we have peer-depends on it.

edmorley added 3 commits Oct 10, 2018
To try and prevent the confusion caused when users do things like:
* using `@neutrinojs/react` followed by `@neutrinojs/web`, which
  clobbers the React parts of the config:
  #1129 (comment)
* using `@neutrinojs/react` followed by `@neutrinojs/style`, which
  causes incorrect `extract` and `hot` configuration:
  #1129 (comment)
  #755 (comment)
  #803 (comment)
  #869 (comment)
@edmorley edmorley force-pushed the edmorley:duplicate-middleware-checks branch from ada126c to 00809b4 Oct 12, 2018
@edmorley

This comment has been minimized.

Copy link
Member Author

edmorley commented Oct 12, 2018

PR updated with a new ConfigurationError and DuplicateRuleError

@edmorley edmorley merged commit c0961d6 into neutrinojs:master Oct 12, 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:duplicate-middleware-checks branch Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.