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

Fix issues with inlined files output by BuildBundler stream #170

Merged
merged 5 commits into from
Apr 17, 2017

Conversation

usergenic
Copy link
Contributor

@justinfagnani
Copy link
Contributor

@usergenic can you update to pull in Polymer/polymer-bundler#464 ?

@justinfagnani justinfagnani added this to the 0.10.0 milestone Apr 9, 2017
@justinfagnani justinfagnani self-requested a review April 9, 2017 04:18
@usergenic
Copy link
Contributor Author

Updated and ready for review! PTAL.

/cc @azakus this is the fix you're looking for wrt duplicates.

@usergenic
Copy link
Contributor Author

(just pushed --force after rebase because master was so far ahead.)

@dfreedm
Copy link
Member

dfreedm commented Apr 17, 2017

@usergenic yep, that's about what I did on my side, so it should fix my issue. Thanks!

Copy link
Contributor

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

:( that the bundler now blocks stream throughput, but 👍 👍 👍 towards fixing these two major bugs. Good stuff.

@usergenic Do you know which users have complained about this specifically, maybe in that issue? It would be good to get them as beta testers before a greater launch, since bundling is such a crucial piece of build and are tests still aren't entirely complete.

src/bundle.ts Outdated
});
this._mapFile(file);
}
for (const filepath of this.files.keys()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can just do this instead:


for (const file of myMap.values()) {
  this.push(file);
}

or even:

this.files.forEach((file) => this.push(file));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! Done.

_unmapBundledFiles(manifest: BundleManifest) {
for (const bundle of manifest.bundles.values()) {
for (const filename of bundle.files) {
this.files.delete(filename);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't comment on the this.files definition above, but please document what this.files state represents. In all other streams it represents a cache of all files, but now it's contents actually dictate the stream output. This would be useful to explain somewhere for later readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added documentation for property.

@usergenic usergenic merged commit 520a829 into master Apr 17, 2017
@usergenic usergenic deleted the everything-once branch April 17, 2017 21:47
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.

None yet

4 participants