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

Inline setProperty function #1122

Merged
merged 2 commits into from May 27, 2018

Conversation

andrewiggins
Copy link
Member

Inlining the setProperty function (which is only used once) saves 7 bytes. Thought I could try to pay back some of the bytes of new features that have gone into Preact recently.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 Makes me wonder if we can inline more functions throughout our code base. Inlining seems to save quite a few bytes 👍

src/dom/index.js Outdated
// Attempt to set a DOM property to the given value.
// IE & FF throw for certain property-value combinations.
try {
node[name] = value==null ? '' : value;
Copy link
Member

Choose a reason for hiding this comment

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

Two spaces or a tab after the equals sign :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops - amended my commit with the fix

Inlining the setProperty function (which is only used once) saves 7 bytes.
@coveralls
Copy link

coveralls commented May 27, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 6887680 on andrewiggins:inline-set-property into fd62f6a on developit:master.

@marvinhagemeister marvinhagemeister merged commit 1b898e2 into preactjs:master May 27, 2018
@marvinhagemeister
Copy link
Member

Thanks again for your contribution 🎉 💯

@andrewiggins andrewiggins deleted the inline-set-property branch May 28, 2018 22:34
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

3 participants