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 the plugin return object for keep-enabled gzipping #19

Merged

Conversation

dannyfallon
Copy link
Contributor

In v0.2.0 we changed the return object for the plugin so that when
the keep option is enabled we set a distFiles value in addition
to gzippedFiles. This ensured our gzipped files got picked up by
later plugins (e.g the manifest, S3 upload etc).

Javascript passes objects by reference. You can see in the result
object we were using the same gzippedFiles array object returned
from the _gzipFiles Promise map. This turned out to be a bad idea.

Setting distFiles equal to the array of gzipped files works because
the return object from each plugin in the pipeline is merged with
the pipeline's context, effectively concatenating the pipeline's
distFiles with our gzippedFiles array. So far, so logical.

The merge of the result with the existing context object is
performed using lodash's merge function. This function maintains
a stack of object values already merged to avoid cyclical object
merging issues. As each key in the object to be merged is processed
lodash looks up the key's value in the stack. If it has been seen
before then the merge is cut short and the merge result's value for
the given key is set to the value of the merge from the earlier key..

What this means for the gzip plugin is that after the result object
is merged into the existing context by Ember CLI Deploy's pipeline
model, context.distFiles === context.gzippedFiles - they are the
exact same object. This is obviously incorrect and has repercussions
later on e.g. the S3 plugin ends up setting Content-Encoding: gzip
on every file it uploads.

This would have been a whole lot easier to spot if I reordered the
keys in the result object during my initial PR i.e. if I put
distFiles below gzippedFiles. In that case the merge would end
up destroying the context's distFiles and both would just contain
the list of gzipped files, wrecking the pipeline 😒

I've verified the problem still happens with Lodash 4.5.1 so I'm
going to go talk to them. We're going to sidestep the problem here
by not using the same object for both result keys, using a simple
concat to build a new object.

In v0.2.0 we changed the return object for the plugin so that when
the `keep` option is enabled we set a `distFiles` value in addition
to gzippedFiles. This ensured our gzipped files got picked up by
later plugins (e.g the manifest, S3 upload etc).

Javascript passes objects by reference. You can see in the result
object we were using the same `gzippedFiles` array object returned
from the _gzipFiles Promise map. This turned out to be a bad idea.

Setting distFiles equal to the array of gzipped files works because
the return object from each plugin in the pipeline is merged with
the pipeline's context, effectively concatenating the pipeline's
`distFiles` with our `gzippedFiles` array. So far, so logical.

The merge of the result with the existing context object is
performed using lodash's `merge` function. This function maintains
a stack of object values already merged to avoid cyclical object
merging issues. As each key in the object to be merged is processed
lodash looks up the key's value in the stack. If it has been seen
before then the merge is cut short and the merge result's value for
the given key is set to the value of the merge from the earlier key..

What this means for the gzip plugin is that after the result object
is merged into the existing context by Ember CLI Deploy's pipeline
model, `context.distFiles` === `context.gzippedFiles` - they are the
exact same object. This is obviously incorrect and has repercussions
later on e.g. the S3 plugin ends up setting `Content-Encoding: gzip`
on every file it uploads.

This would have been a whole lot easier to spot if I reordered the
keys in the result object during my initial PR i.e. if I put
`distFiles` below `gzippedFiles`. In that case the merge would end
up destroying the context's `distFiles` and both would just contain
the list of gzipped files.

I've verified the problem still happens with Lodash 4.5.1 so I'm
going to go talk to them. We're going to sidestep the problem here
by not using the same object for both result keys, using a simple
concat to build a new object.
@achambers
Copy link
Member

Thanks @dannyfallon. Sounds like that was a but of a pain to track down. Thanks so much for the effort.

achambers pushed a commit that referenced this pull request Feb 26, 2016
Fix the plugin return object for keep-enabled gzipping
@achambers achambers merged commit 4cbb865 into ember-cli-deploy:master Feb 26, 2016
@lukemelia
Copy link
Collaborator

Great job following up on this @dannyfallon! Thank you.

@achambers
Copy link
Member

I'll get a release of this and the lightening plugin pack out today.

@dannyfallon
Copy link
Contributor Author

Please excuse the novel of a PR comment, I felt like I had to write it down to avoid a case of wtf!? if anybody reviews this in the future. It was a bit of a pain to track down but thanks for the swift merge 👍

@achambers
Copy link
Member

Mate, the novel perfectly explained the problem to my simple brain so it was much appreciated. Thank you for spending the time on it.

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.

3 participants