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

[dynamic-import-chunkname] Disallow chunk name when eager mode is enabled #3004

Merged
merged 1 commit into from
May 16, 2024

Conversation

amsardesai
Copy link
Contributor

@amsardesai amsardesai commented Apr 24, 2024

The dynamic-import-chunkname rule is useful for catching missing chunk names for imports, however much of our imports use webpackMode: "eager" to disable creating a separate chunk (i.e for lazy evaluation). Adding webpackChunkName anyway can force webpack to generate a chunk, defeating the purpose of the eager mode. This PR changes the rule to disallow chunk names when webpackMode: "eager" is set.

Related issue: #1904

@ljharb
Copy link
Member

ljharb commented Apr 24, 2024

When would you not want this option enabled?

@amsardesai
Copy link
Contributor Author

Never, really. I'll just make it the default.

…Mode: 'eager' is set; add suggestions to remove name in eager mode'
@amsardesai amsardesai changed the title Add allowEagerMode option to dynamic-import-chunkname rule [dynamic-import-chunkname] Make chunk name optional when eager mode is enabled Apr 24, 2024
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.87%. Comparing base (c0ac54b) to head (48c921c).

❗ Current head 48c921c differs from pull request most recent head a3a7176. Consider uploading reports for the commit a3a7176 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3004      +/-   ##
==========================================
+ Coverage   95.66%   95.87%   +0.20%     
==========================================
  Files          78       78              
  Lines        3275     3293      +18     
  Branches     1150     1156       +6     
==========================================
+ Hits         3133     3157      +24     
+ Misses        142      136       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ljharb
Copy link
Member

ljharb commented Apr 24, 2024

Is there anyone who this could break? I assume not, because the only complaint would be that it fails to warn on something?

@amsardesai
Copy link
Contributor Author

Exactly, this only suppresses existing warnings in a narrow case, and doesn't introduce new warnings.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is great!

One more enhancement we may want to consider before landing this: when webpackMode is eager, and a useless name is present, could we give a suggestion to remove the name?

@amsardesai
Copy link
Contributor Author

@ljharb Yeah that makes sense to me, just updated the PR to warn when chunk name and eager mode are both set.

@ljharb
Copy link
Member

ljharb commented May 2, 2024

@amsardesai sorry if i wasn't clear; a "suggestion" isn't prose giving the user a hint, it's an actual eslint feature where the editor will prompt them with multiple autofixes they have to explicitly choose. For an example, look for a rule with hasSuggestions: true in its schema.

@amsardesai
Copy link
Contributor Author

amsardesai commented May 2, 2024

Ah, I didn't realize you meant ESLint suggestions. Let me do that.

@amsardesai amsardesai changed the title [dynamic-import-chunkname] Make chunk name optional when eager mode is enabled [dynamic-import-chunkname] Disallow chunk name when eager mode is enabled May 2, 2024
@amsardesai
Copy link
Contributor Author

Hey @ljharb, would love to know if this can be merged now, or if any other changes need to be made!

@ljharb
Copy link
Member

ljharb commented May 10, 2024

@amsardesai ok, i've rebased this and it's probably ready to go :-) (assuming tests pass) just wondering about some of the remaining test changes (not the additions, those make sense) - some are lazy → eager, and some are eager → lazy. Can you go through and comment in the PR on those lines, and explain what motivated the change? i suspect you have a convincing explanation for them all but this way it'll be documented for me in the future :-)

Copy link
Contributor Author

@amsardesai amsardesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also updated some tests to be a bit more thorough!

tests/src/rules/dynamic-import-chunkname.js Show resolved Hide resolved
tests/src/rules/dynamic-import-chunkname.js Show resolved Hide resolved
tests/src/rules/dynamic-import-chunkname.js Show resolved Hide resolved
tests/src/rules/dynamic-import-chunkname.js Show resolved Hide resolved
@ljharb ljharb merged commit a3a7176 into import-js:main May 16, 2024
309 checks passed
@amsardesai amsardesai deleted the ankit/support-eager-mode branch May 22, 2024 21:30
@amsardesai
Copy link
Contributor Author

Hey @ljharb, would love to know when you will put out a new release - I'm hoping to use the changes in this PR in our repo.

@ljharb
Copy link
Member

ljharb commented Jun 17, 2024

I was hoping to get flat config support landed before cutting a release; I'll plan to cut one within the next couple of weeks regardless.

@amsardesai
Copy link
Contributor Author

Thanks for the update!

@amsardesai
Copy link
Contributor Author

Hey @ljharb, just pinging again to check if you will release soon, since it's been 8 months since the last release.

@ljharb
Copy link
Member

ljharb commented Aug 28, 2024

@amsardesai i'll probably have to cut a release without flat config support, sadly, so i'd expect one in the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants