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

[WIP] Make things a bit classier #1359

Closed
wants to merge 9 commits into from
Closed

[WIP] Make things a bit classier #1359

wants to merge 9 commits into from

Conversation

Rich-Harris
Copy link
Member

Just an idea for now, it may turn out that this costs us more than it gains us. But now that we've dropped the ES5 constraint we can use classes to generate (in my opinion) much nicer code.

Custom elements are currently broken, and we'd need to handle the case where options.name is Component, since it doesn't currently deconflict.

Also, I can't think of a good name for the base class shared by Component and Store. Anyone?

@rhengles
Copy link

  • SvelteObject
  • AbstractComponent
  • BaseComponent
  • EventHandler
  • EventEmitter (like Node)

Looking at the source... if you remove the get() and _differs() it is all about event handlers. Maybe you could do this? Extract the event functions into a EventEmitter, and create another class AbstractComponent extending that and adding the get() and _differs() methods ?

@Rich-Harris
Copy link
Member Author

In the interests of keeping byte size to a minimum, it's probably better to keep get and _differs where they are. But your answers made me think of Base, which is probably as good a name as any. Will change it to that

@codecov-io
Copy link

codecov-io commented Apr 21, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e0fe313). Click here to learn what that means.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1359   +/-   ##
=========================================
  Coverage          ?   92.09%           
=========================================
  Files             ?      127           
  Lines             ?     4958           
  Branches          ?     1592           
=========================================
  Hits              ?     4566           
  Misses            ?      158           
  Partials          ?      234
Impacted Files Coverage Δ
src/shared/utils.js 100% <ø> (ø)
src/generators/Generator.ts 97.47% <100%> (ø)
src/generators/dom/index.ts 95.18% <93.33%> (ø)

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 e0fe313...6cb9155. Read the comment docs.

@Rich-Harris
Copy link
Member Author

I like the code this results in way more, though some discouraging initial news — the Hello World example is slightly larger:

  • 2.1.0 — 2252 bytes, 987 bytes zipped
  • This PR — 2287 bytes, 1003 bytes zipped
  • This PR with Bublé transpilation — 3262 bytes, 1282 zipped

Also, custom elements are slightly clunky.

There might still be a couple of improvements we can make, and it needs to be tested with some larger components.

@Rich-Harris
Copy link
Member Author

Rich-Harris commented Apr 21, 2018

Ok, so I just ran this on svelte.technology, and got slightly better results:

  • 2.1.0 — 498,625 bytes (all routes concatenated), 129,605 zipped
  • This PR — 497,644, 128,649 zipped 497,027, 128,518 zipped 495,019, 127,960 zipped

A modest saving but a saving nonetheless. So the question is whether (nicer code + smaller ES6 output) is a more important consideration than (larger transpiled ES5 output).

@tivac
Copy link
Contributor

tivac commented Apr 22, 2018

Does perf change at all?

@Rich-Harris
Copy link
Member Author

Lemme get back to you on that. Running the js-framework-benchmarks, it takes a while...

@Rich-Harris
Copy link
Member Author

Here's the results: https://file-oprglutiur.now.sh/

There's basically nothing to choose between them. This PR ('linked') is the slowest of three on the keyed tests, fastest on the non-keyed, but they're so close that I'd call that statistical noise rather than anything. Perhaps not surprising, since it's only actually instantiating a single component.

Incredibly though, this micro-benchmark I just threw together suggests that while classes are faster to instantiate than constructors in Chrome, and constructors are faster than classes in Firefox, Object.create() trounces both of them. Maybe we should be using that everywhere instead?

@tivac
Copy link
Contributor

tivac commented Apr 22, 2018

Interesting, I wouldn't have expected that at all! Might be worth trying if you're down for the extra work.

@mrkishi
Copy link
Member

mrkishi commented Apr 23, 2018

@Rich-Harris I'd take those micro-benchmarks with a grain of salt. It really depends on what we're optimizing for.

We get different results when instantiating multiple components (eg. by moving the classes definitions to the setup); also when disabling the babel transpilation; and I bet it'd be even more varied if we tested long and short-lived components after creation (since all engines support hidden classes nowadays, and I'm not sure if any of them optimize Object.create yet).

ps. ESBench is failing on me so I can't save any of the modified benchmarks, sorry!

This was referenced May 4, 2018
@tomcon
Copy link

tomcon commented Jul 25, 2018

Any update on this please @Rich-Harris?
Given the use of TS, was wondering how fired up you were on this (with your limited time). It also might make understanding the code that bit easier for others once implemented perhaps??

@Rich-Harris
Copy link
Member Author

Closing; v3 uses classes

@Conduitry Conduitry deleted the classy branch April 29, 2019 11:37
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.

6 participants