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 check chunk-name format only in dynamic-import-chunkname rule #2941

Closed
JiangWeixian opened this issue Dec 15, 2023 · 18 comments · Fixed by #2942
Closed

support check chunk-name format only in dynamic-import-chunkname rule #2941

JiangWeixian opened this issue Dec 15, 2023 · 18 comments · Fixed by #2942

Comments

@JiangWeixian
Copy link
Contributor

Thanks for awesome works :)

Currently dynamic-import-chunkname rule require webpack chunk name, will report error if not exit.

Is able to add a option to check chunk-name format only, if webpack chunk name not exit is ok.

Currently dynamic-import-chunkname will throw error for

// Bad
import("someModule")

// Good
import(
    /* webpackChunkName: "someModule" */
    'someModule',
  );

// Bad
import(
  /*webpackChunkName:"someModule"*/
  'someModule',
);

I want to add a option to make

// Good
import("someModule")

// Bad
import(
  /*webpackChunkName:"someModule"*/
  'someModule',
);

I glad like open a PR for this feature.

Related un-ts#21

@ljharb
Copy link
Member

ljharb commented Dec 15, 2023

You want to forbid webpack chunk name comments?

@JounQin
Copy link
Collaborator

JounQin commented Dec 15, 2023

@ljharb
Copy link
Member

ljharb commented Dec 15, 2023

That’s the default behavior of the language without the rule enabled tho.

@JounQin
Copy link
Collaborator

JounQin commented Dec 15, 2023

I think he wants to allow empty leadingComments but check the format if it exists?

@ljharb
Copy link
Member

ljharb commented Dec 15, 2023

Hmm, ok - what's the use case? When would you want some dynamic imports handled by webpack but some not?

@JounQin
Copy link
Collaborator

JounQin commented Dec 15, 2023

@ljharb

Take the following as example, when dynamic string included, a chunk name is good to have.

import(
  /* webpackChunkName: "my-chunk-name" */
  `./locale/${language}`
);

But then the expression itself is static, no need to change it.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2023

The staticness or dynamicness of the import specifier isn't relevant to webpack tho. This rule was created because in a codebase that's using webpack dynamic import code splitting, every single dynamic import needs to have a chunk name identified.

@JiangWeixian
Copy link
Contributor Author

JiangWeixian commented Dec 16, 2023

@ljharb

When I wrap dynamic import with loadable, e.g.

loadable(() => import("some-module"))

it provides a babel-plugin or swc-plugin to automatic add webpackChunkName during compile time(or swc transform).

automatic generate chunkName only enabled when webpackChunkName not found in leadingComments I think it's ok to allow empty leadingComments

@ljharb
Copy link
Member

ljharb commented Dec 16, 2023

If you’re using that plugin, why would you use this rule?

@JiangWeixian
Copy link
Contributor Author

JiangWeixian commented Dec 16, 2023

If use these plugins:

// will automatic add webpackChunkName, /* webpackChunkName: "foo" */
loadable(() => import("foo"))

// will not automatic add webpackChunkName,  /* webpackChunkName: "foo" */, use /* webpackChunkName: "some-module" */  instead.
loadable(() => import(/* webpackChunkName: "some-module" */ "foo"))

Because this rule also check webpackChunkName format. If I already defined webpackChunkName, I want to check webpackChunkName format is right or not. Webpack split chunks not working on wrong magic webpackChunkName commets syntax.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2023

OK, thanks, that's at least a use case :-) I'll wait til I hear @JiangWeixian's use case before I decide anything tho.

@JounQin
Copy link
Collaborator

JounQin commented Dec 16, 2023

OK, thanks, that's at least a use case :-) I'll wait til I hear @JiangWeixian's use case before I decide anything tho.

Not sure what's your meaning, @JiangWeixian has already provided his use case.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2023

I don't see where; the OP only asks for a solution. A use case is the "why" - the problem they're having and why they're having it.

@JounQin
Copy link
Collaborator

JounQin commented Dec 16, 2023

OK, thanks, that's at least a use case

What does this mean?

@ljharb
Copy link
Member

ljharb commented Dec 16, 2023

It means that you provided the first motivation - reason/justification - for making any changes here, since the OP still hasn't provided any.

@JounQin
Copy link
Collaborator

JounQin commented Dec 16, 2023

image

I think you meant the OP here? I'm not requesting this feature, just help the OP to explain. I don't have such usage case.

@ljharb
Copy link
Member

ljharb commented Dec 16, 2023

ah thanks, i got confused :-)

In that case, an option that ignores missing comments would be fine.

@JounQin
Copy link
Collaborator

JounQin commented Dec 16, 2023

@JiangWeixian So this feature is ready to be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants