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

Why force specific quotes in dynamic-import-chunkname? #1130

Closed
st-sloth opened this issue Jul 4, 2018 · 11 comments · Fixed by #1848
Closed

Why force specific quotes in dynamic-import-chunkname? #1130

st-sloth opened this issue Jul 4, 2018 · 11 comments · Fixed by #1848

Comments

@st-sloth
Copy link
Contributor

st-sloth commented Jul 4, 2018

The rule dynamic-import-chunkname forces to use double quotes in the chunk comment. Is is literally said in docs so:

invalid

The following patterns are invalid:

// using single quotes instead of double quotes
import(
  /* webpackChunkName: 'someModule' */
  'someModule',
);

What is the rationale for that? Should I make a PR allowing both single and double quotes by default, or put allowing / forbidding behind an option?

@ljharb
Copy link
Member

ljharb commented Jul 4, 2018

Does webpack support both formats?

@st-sloth
Copy link
Contributor Author

st-sloth commented Jul 4, 2018

It does. Moreover it supports a couple of other "invalid" cases:

// incorrectly formatted comment
import(
  /*webpackChunkName:"someModule"*/
  'someModule',
);

// single-line comment, not a block-style comment
import(
  // webpackChunkName: "someModule"
  'someModule',
);

because it "parses" these options by inserting comment contents inside an object body. Which we could also do here in this rule, that is currently too strict and considers cases with multiple "comment options", like webpackChunkName: "someModule", anotherOption: 123 invalid because of lack of a space after the closing quote. (I guess I should have named the issue after overall specificity of the rule in the first place, not just quotes). So, do I migrate the parsing logic here?

@ljharb
Copy link
Member

ljharb commented Jul 4, 2018

I’m fine adding an option for quotes, but i don’t think there’s any value in allowing “anything”

@st-sloth
Copy link
Contributor Author

st-sloth commented Jul 5, 2018

  1. By option you mean "default is any + restrictive option" or "default stays double + relaxative option?

  2. In general, perhaps having comment parsing logic different from that of webpack isn't the best approach? Because, for instance, parsing multiple comment options is currently broken. It reports as invalid proper syntax:

import(
    /* webpackChunkName: "a" */
    /* webpackPrefetch: true */
    './a'
)

import(
    /* webpackPrefetch: true */
    /* webpackChunkName: "a" */
    './a'
)

import(/* webpackChunkName: "a", webpackPrefetch: true */ './a')

while accepting

import(/* webpackPrefetch: true, webpackChunkName: "a" */ './a')

import(/* totally invalid syntax webpackChunkName: "a" !!!! */ './a')

because of current substring search with single spaces around.

Second point might be considered separate issue, but everything discussed here boils down to the comment parsing logic. But the rule is about

dynamic imports without a webpackChunkName specified in a leading block comment in the proper format

so it needs to both parse correctly and enforce some whitespace styling.

@ljharb
Copy link
Member

ljharb commented Jul 5, 2018

To be non-breaking, the default would stay double, and an option could be added to require single instead of double. It wouldn't be very valuable to allow either :-)

The rule should certainly allow all of webpack's features - but i don't think it's a good idea to be as loose as webpack is in a linter rule.

@st-sloth
Copy link
Contributor Author

In the meantime, I re-evaluated the position of customizing these little details, and don't think bringing in options for the quotes and spacing preferences adds enough value to even cover the added code complexity and maintenance burden.

After all, this rule's main goal is enforcing naming of webpack chunks in dynamic imports, and "the proper format" is only second to that (if at all).

Personally, I think simply allowing both quote styles (which is relaxing and non-breaking) would be more beneficial than forcing a specific quote style or having an option for which to enforce.
But if you are against that, what do you think if I make a PR to update the docs to mention the reasoning behind what is considered "the proper format", to hopefully prevent further questions and spending time of both the team members and developers using the rule?

@ljharb
Copy link
Member

ljharb commented Feb 16, 2019

Updating the docs is of course always great. I agree there’s added complexity in an option, but in this case it’d just be selecting a single or double quote - it doesn’t seem that painful to add to me.

@ljharb
Copy link
Member

ljharb commented Mar 6, 2019

I believe #1163 addressed this.

@straub
Copy link
Contributor

straub commented Jul 7, 2020

@ljharb It looks like #1163 omitted allowing single quotes:

The main topic of #1130 - allowing single quotes in strings in webpack comments - will be implemented on top of these changes once they are merged.

Is there still interest in relaxing the double quote requirement? If so, I'd be more than happy to contribute a PR!

@ljharb
Copy link
Member

ljharb commented Jul 7, 2020

Yes, absolutely! As long as webpack supports it, a PR would be great (please confirm in which versions webpack supports it, in the PR)

@straub
Copy link
Contributor

straub commented Jul 7, 2020

Took a pass at it in #1848, thanks!

ljharb pushed a commit to straub/eslint-plugin-import that referenced this issue Aug 30, 2020
bogdanb pushed a commit to bogdanb/eslint-plugin-import that referenced this issue Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants