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

[internals] Inline helpers + tweaks #4923

Closed
wants to merge 10 commits into from

Conversation

pushkine
Copy link
Contributor

@pushkine pushkine commented May 28, 2020

$$restProps & $$slots

  • inline all helpers but get_slot_changes
  • removed unused $$scope declaration variable in dev mode

render_listeners

  • following domnodes pattern, each listener now gets its own variable
  • avoid declaring an extra variable for action.destroy

other helpers inlined at compile time

  • destroy_each( arr, detaching )
    • for ( ... ) if ( each_value ) each_value.d( detaching )
  • query_selector_all( id, document.head )
    • Array.from((document.head || document.body ).querySelectorAll( id ))
  • create_component / claim_component (component.$$.fragment)
    • if ( component.$$.fragment ) component.$$.fragment .c() / .l()
  • component_subscribe( $$self, store, callback )
    • $$self.$$.on_destroy.push(subscribe(store, callback))
  • null_to_empty
    • null != value ? value : ""
  • is_function
    • "function" === typeof value
  • is_promise
    • value && "object" === typeof value && "function" === typeof value.then
  • assign
    • { ...a, ...b }
  • blank_object
    • Object.create( null )
  • get_spread_object
    • typeof props === "object" && props !== null ? props : {}
  • run / run_all
    • for( ... ) each_value();
  • foo = get_store_value( store )
    • subscribe(store, (v) => foo = v)()

src/runtime

  • inlined components update function
  • moved all dev functions to dev.ts
  • predeclared loop variables & removed the unnecessary consts in dom.ts and spread.ts
  • moved once modifier from utils.ts to dom.ts
  • converted some .forEach into for loops

Those changes naturally require updating expected test results. I intentionally did not commit those so as to give maintainers an opportunity to test the #4914 script, but all test are passing.

@pngwn
Copy link
Member

pngwn commented May 28, 2020

Can I ask why these changes are necessary? What benefits do they bring?

@pushkine
Copy link
Contributor Author

Can I ask why these changes are necessary? What benefits do they bring?

Hiya!
My understanding is that the use of helpers, methods & iterations to type less code and improve readability is worth it in user land, however in tools such as in a compiler the goal is rather to take every chance you get to "pre chew" any piece of work before it lands in the browser

I don't know, is there a disagreement or a misunderstanding ?
Don't hesitate hitting me up on discord for instant messaging btw

@pushkine
Copy link
Contributor Author

pushkine commented May 28, 2020

The disagreement appears to be ( obviously ) about the tradeoff between extra bytes & runtime performance
I guess it means the PR is on sleep until we get some kind of benchmarking and sciency graph going

My bet is that nobody really knows since the source code actually has strong yet irresolute opinions about this very subject, it really is swinging real hard one way and the other at times

@tanhauhau
Copy link
Member

Hmm.. i believe this PR (the beginning of the helper functions) is something worth look at before you start working on this.

The helpers weren't added because of readability, it was for better minifiable.

@tanhauhau
Copy link
Member

tanhauhau commented May 29, 2020

My bet is that nobody really knows since the source code actually has strong yet irresolute opinions about this very subject

I do understand why you get this from the source code.

Inline code gets bigger and bigger with time, and the general directions is to move them into helpers, to make the compiler code simpler as well as the output more minifiable, for example: #1256 moved keyed each diffing into a shared helper.

So I am going to close this PR since it is mainly about inlining helpers. It is moving towards an opposite direction to what we are heading.

(moving validate_component, validate_store, validate_each_keys into dev.ts might be a good idea of grouping them, if you are going to do it, i think you should open a separate PR for that.)

@tanhauhau tanhauhau closed this May 29, 2020
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