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

Performance boost for Assign utility #1282

Merged
merged 5 commits into from
Mar 27, 2018
Merged

Conversation

lukeed
Copy link
Member

@lukeed lukeed commented Mar 27, 2018

I believe the current assign utility was my doing anyway 😆

This new one is borrowed from Preact. While its usage for multiple assignments feels a bit clunky, its perf boost is still significant, and minifies to a smaller footprint overall.

screen shot 2018-03-26 at 8 42 15 pm

Source: ESBench

@Conduitry
Copy link
Member

Conduitry commented Mar 27, 2018

What do you think about an assignDev version that throws if it gets a third argument? Increased perf is good but this looks like the sort of change that could result in hair-pulling later on :)

edit: Hm I'm actually not positive that the framework is already in place for that. We can have dev versions of things that end up on the component prototype, but maybe not for other utilities. Nope I think we should be good there.

@lukeed
Copy link
Member Author

lukeed commented Mar 27, 2018

Well, it'd definitely be a breaking change since it's accessible to the user. I'm working on the code changes to make tests pass -- at this point I think all I need is state to work.

@Conduitry
Copy link
Member

Well, it'd definitely be a breaking change since it's accessible to the user.

I'm pretty sure svelte/shared.js is not considered part of the API. Its usage is not documented anywhere, and we've changed it in the past without it being a breaking change. You always need to use the same version of shared.js as you used to compile your components.

@lukeed
Copy link
Member Author

lukeed commented Mar 27, 2018

Ah right -- not breaking but worth mentioning for the few who may be importing from shared.

I forgot how thorough the test suite is... goals. 💯👏

@codecov-io
Copy link

codecov-io commented Mar 27, 2018

Codecov Report

Merging #1282 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1282      +/-   ##
=========================================
- Coverage   91.51%   91.5%   -0.01%     
=========================================
  Files         121     121              
  Lines        4326    4322       -4     
  Branches     1361    1361              
=========================================
- Hits         3959    3955       -4     
  Misses        153     153              
  Partials      214     214
Impacted Files Coverage Δ
src/generators/nodes/EachBlock.ts 97.6% <ø> (ø) ⬆️
src/generators/nodes/AwaitBlock.ts 92.3% <ø> (ø) ⬆️
src/shared/utils.js 100% <100%> (ø) ⬆️
src/generators/dom/index.ts 96.04% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcfbffe...69ba60b. Read the comment docs.

@Rich-Harris Rich-Harris merged commit 6a15ebd into sveltejs:master Mar 27, 2018
@Rich-Harris
Copy link
Member

brent-rambo.gif

@lukeed lukeed deleted the fix/assign branch March 27, 2018 21:45
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.

4 participants