Skip to content

Fix a minification issue with the __hasRegisterFinished property.#5584

Merged
kevinpschaaf merged 2 commits intomasterfrom
closure-__hasRegisterFinished
Aug 27, 2019
Merged

Fix a minification issue with the __hasRegisterFinished property.#5584
kevinpschaaf merged 2 commits intomasterfrom
closure-__hasRegisterFinished

Conversation

@bicknellr
Copy link
Copy Markdown
Member

Wrap hasOwnProperty checks for __hasRegisterFinished in JSCompiler_renameProperty().

Copy link
Copy Markdown
Member

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

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

While we're at it, I just searched the code and there are a few other hasOwnProperty checks with string literals that should be wrapped in JSCompiler_renameProperty:

  • properties-changed.js: __dataHasAccessor
  • properties-changed.js: __dataAttributes
  • properties-mixin.js: __observedAttributes

I spot checked these in compiled code and I don't see them being renamed (not entirely sure why), but for safety, let's go ahead and close the hole.

@bicknellr
Copy link
Copy Markdown
Member Author

I've never actually used JSCompiler_renameProperty and I wasn't able to find any documentation about the two-argument form, but from other uses I gathered that the second argument was meant to be the thing that you're going to pull the property from (i.e. thing[JSCompiler_renameProperty('propName', thing)]). Is that correct?

@bicknellr bicknellr requested a review from kevinpschaaf August 27, 2019 18:29
Copy link
Copy Markdown
Member

@kevinpschaaf kevinpschaaf left a comment

Choose a reason for hiding this comment

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

Yeah that's right, 2nd arg is the context object for the property being dereferenced.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants