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

fix: migrate entry injection to hooks #3424

Closed
wants to merge 4 commits into from
Closed

Conversation

rishabh3112
Copy link
Member

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Shouldn't require change

Motivation / Use-Case

/cc @alexander-akait
Helps fixing #3351

Breaking Changes

No

Additional Info

No

@snitin315
Copy link
Member

Need to rebase.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #3424 (30eaf1b) into master (2b53b00) will decrease coverage by 2.38%.
The diff coverage is 81.56%.

❗ Current head 30eaf1b differs from pull request most recent head e1daeff. Consider uploading reports for the commit e1daeff to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3424      +/-   ##
==========================================
- Coverage   95.24%   92.86%   -2.39%     
==========================================
  Files          34       33       -1     
  Lines        1326     1317       -9     
  Branches      392      405      +13     
==========================================
- Hits         1263     1223      -40     
- Misses         59       85      +26     
- Partials        4        9       +5     
Impacted Files Coverage Δ
bin/cli-flags.js 100.00% <ø> (ø)
client-src/index.js 83.18% <57.14%> (-7.89%) ⬇️
lib/utils/findCacheDir.js 61.90% <61.90%> (ø)
lib/utils/DevServerPlugin.js 84.39% <74.66%> (-12.80%) ⬇️
lib/servers/SockJSServer.js 90.69% <77.77%> (-4.04%) ⬇️
lib/Server.js 95.23% <97.77%> (+0.21%) ⬆️
client-src/clients/SockJSClient.js 100.00% <100.00%> (ø)
client-src/clients/WebsocketClient.js 100.00% <100.00%> (+11.76%) ⬆️
lib/servers/WebsocketServer.js 97.36% <100.00%> (+0.30%) ⬆️
lib/utils/getCertificate.js 88.46% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b53b00...e1daeff. Read the comment docs.

@rishabh3112
Copy link
Member Author

Need review 😃

}).apply(compiler);
}
// Register Entry Dependency in Factory
const EntryDependency = require('webpack/lib/dependencies/EntryDependency');
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using this, webpack should public export this

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I send PR to webpack then?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ylemkimon
Copy link
Contributor

Note this is no longer needed as of #3467.

@rishabh3112
Copy link
Member Author

Closing this PR.
@alexander-akait if this is needed, let me know. I would start fresh.

@rishabh3112 rishabh3112 closed this Sep 8, 2021
@rishabh3112 rishabh3112 deleted the fix/migrate-to-hook branch September 8, 2021 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants