Skip to content
This repository has been archived by the owner on Jan 21, 2021. It is now read-only.

Provide a way to include all assets (issue #35) #36

Merged
merged 11 commits into from
Feb 28, 2018

Conversation

laysent
Copy link
Contributor

@laysent laysent commented Sep 6, 2017

This PR tries to add feature to add all assets for plugin, not just chunks. In this way, resources handled by file-loader can also be added (#35).
Also, when I tried this version in my own project, I notice that it might be a bit annoying to use fileBlacklist to exclude every type of files that I don't want to include (too many assets). Thus, I also create an option named fileWhitelist to include resources. That might make things easier in some situation.

One thing I noticed is that file-loader provides an option to specify publicPath just for assets. However, that information seems to be lost when this plugin is doing its job. In code, it is using publicPath defined in output. It could be an issue, if these publicPath have different values. However, I am not able to find a way to work around this. Any hint will be very welcome.

Copy link

@jarrybomb jarrybomb left a comment

Choose a reason for hiding this comment

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

thank yo
u

@madmoizo madmoizo mentioned this pull request Dec 28, 2017
@umpc
Copy link

umpc commented Jan 28, 2018

Since a best solution to the question about differing publicPath values has not yet been found:

Would it be acceptable to add a note related to that behavior to the Readme and merge this PR in the meantime?

Someone who wants to use this functionality, and has that issue, could open a new issue that hopefully includes a proposed solution that works best for that use-case.

I say this because I'm eagerly awaiting this functionality in the npm module, and would prefer not to keep track of when this is added. I doubt that I'm the only one working around this. 😁

I thank you, and the reviewer, for the work done on this feature!

Edit: I made a fork, merging these changes, and I now see that it can't be merged immediately, contrary to what the above message may have implied.

@laysent
Copy link
Contributor Author

laysent commented Jan 29, 2018

@umpc Thanks for the suggestions. I have updated the README to point out this potential issue. :-)

@GoogleChromeLabs GoogleChromeLabs deleted a comment from jarrybomb Jan 29, 2018
@gkatsanos
Copy link

Is there any plan to merge this?

@alechill
Copy link
Contributor

Agree, would be great to get this merged, but for now tests fail on....

Expected 'Html Webpack Plugin:
    <pre>
      TypeError: chunk.parents is not iterable

      - index.js:40 doesChunkBelongToHTML
        /Users/Alec/workspace/misc/forks/preload-webpack-plugin/index.js:40:30

@laysent Same thing occurs in real use, and seeing as I need to use it now, I forked yours and did a simple fix, PR'd it in your fork in case you want it laysent#1

@gkatsanos
Copy link

I am confused by the documentation/README. Reading it looks like you can already use it for all assets. But in reality it doesn't work? Am I missing something?

…ML - it should not be attempted to filtered as if it were a chunk, they are just files to be black/whitelisted in config
Fixed all-assets being treated as a real chunk in doesChunkBelongToHML
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@laysent
Copy link
Contributor Author

laysent commented Feb 18, 2018

Hi @gkatsanos, @alechill has fixed an issue of loading all chunks, would you mind to try again and see if that works?
@alechill It seems you might need to sign the CLA as well. See above message from googlebot

@gkatsanos
Copy link

@laysent : the fix isn't on master/release yet right?

@alechill
Copy link
Contributor

@laysent Thanks, CLA sorted 👍

@laysent
Copy link
Contributor Author

laysent commented Feb 22, 2018

@gkatsanos nope, it's only on my PR and hasn't been merged yet.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@gkatsanos
Copy link

@laysent alright, I'll wait for it to be merged and test !

@madmoizo
Copy link

There is something to do on the naming:

  • asyncChunks is pascal case when all-assets is kebab-case. Rename one or the other to be consistent
  • all and all-assets will be a source of confusion. maybe rename all in all-chunks (or allChunks)

+ use pascal case: "all-assets" to "allAssets"
+ rename "all" to "allChunks"
@laysent
Copy link
Contributor Author

laysent commented Feb 28, 2018

@frlinw Hi, thanks for the review. I have made it all pascal case.
Also, I changed all to allChunks. To ensure it will not break existing code, all is still available, but not mentioned in README. Also, I add warning when all is used.

@jeffposnick
Copy link
Contributor

Thanks for everyone's patience—I'm going to merge into master now, and will cut a new minor v2 release with this and any other pending changes.

(Please note that we're also thinking about an upcoming v3 release which will be informed by the options available in v2 (including in this PR), but which might revisit some of the decisions around some of the defaults, and the names used in the public interface.)

@jeffposnick jeffposnick merged commit 0f80892 into GoogleChromeLabs:master Feb 28, 2018
@freezy
Copy link

freezy commented Mar 31, 2018

So fileWhitelist didn't make it into v3 yet I suppose?

@JasonBoy
Copy link

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

Successfully merging this pull request may close these issues.

10 participants