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(index): update to allow requesting failed async css files #292

Merged
merged 4 commits into from Nov 21, 2018

Conversation

cwalten
Copy link
Contributor

@cwalten cwalten commented Oct 4, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Related to #278

Since a rejected promise is left in installedCssChunks on a failed async import, the code to add the HTML link tag again is totally skipped.

Further, the code which has been removed in this PR is currently causing dynamically imported CSS files which have failed (but still have the link/ style tag added in the DOM) to simply return a resolve() instead of retrying the request. If we return resolve() simply because the link tag is in the DOM, the request is never retried and, though the Promise resolves correctly, the CSS is not loaded into the page.

Breaking Changes

Yes, as loading behavior of dynamically imported CSS files has been updated. Previous implementations might have been relying on the old functionality, which did not allow for retry scenarios.

Additional Info

These changes are modeled off of the async jsonp template code from webpack found here, where they do not do these duplication checks and reset the chunk key in the installedChunks object when the request fails.

@jsf-clabot
Copy link

jsf-clabot commented Oct 4, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

It is breaking change, also need tests

@cwalten cwalten force-pushed the master branch 2 times, most recently from ff3cf0a to c3bec42 Compare October 5, 2018 14:25
@cwalten
Copy link
Contributor Author

cwalten commented Oct 5, 2018

Okay, thanks for the input. I am working on tests now. I am also having trouble lining up my github account to the CLA form. I have signed it, but my initial commit was linked to a different account accidentally. I have overwritten the commitor and author fields but it is not triggering as expected. I will be working on fixing that after testing.

Edit: added a manual test for this scenario that fails on current master but passes on my fork. Also due to that commit was seemingly able to reset the CLA bot.

@cwalten
Copy link
Contributor Author

cwalten commented Oct 12, 2018

the failed test is also failing in master here

@tbiethman
Copy link

@evilebottnawi @cwalten Any updates regarding the status of this PR? I would like to start consuming these changes when possible.

@alexander-akait
Copy link
Member

/cc @ooflorent @sokra

Copy link
Contributor

@ooflorent ooflorent left a comment

Choose a reason for hiding this comment

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

This change looks good.

I don't know the BC policy of this plugin. If it's okay to break master, then go for it. Otherwise, maybe this PR should land on a next branch…

test/manual/index.html Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@goenning
Copy link

Just for the record, I replaced my mini-css-extract-plugin dependency with cwalten's master and it solved the problem. I can now retry loading CSS files just like it's done with JS files. 👍

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Good work, will be shipped in near future 👍

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

7 participants