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] Dev helpers #378

Merged
merged 5 commits into from
Mar 16, 2017
Merged

[WIP] Dev helpers #378

merged 5 commits into from
Mar 16, 2017

Conversation

Rich-Harris
Copy link
Member

Picking up where @Conduitry left off in #365 (comment) — opening a PR so we can address
#365 and #369

@Conduitry
Copy link
Member

A couple of things that spring to mind - Where we're attaching the methods to the prototype, the second case (when we're not using shared helpers) isn't I believe actually strictly necessary. If we instead did

[ 'get', 'fire', 'observe', 'on', 'set', '_flush' ].forEach( methodName => {
	builders.main.addLine( `${name}.prototype.${methodName} = ${generator.helper( methodName )};` );
});

then we'd generate a normal reference to a helper function, which would then be included later along with the other helper functions. I don't know how important declaring and assigning the methods in one go is. What I have now in the branch does save a couple of bytes in the compiled js, but having the code generation be simpler does seem nice - there has to be some duplicated logic the way it is now,

Another is that the recently-added .set('key', 'value') check that was added to the _set method might be nice to move to the set helper itself, now that we can have a dev version of that.

@Rich-Harris
Copy link
Member Author

That makes sense, yeah. Though I was thinking that in shared mode, maybe the prototype itself could be shared...

import { proto... } from 'svelte/shared.js';

// ...

Component.prototype = Object.assign( {}, proto, template.methods );

This would shrink things down a bit further. Might make sense for component-specific destroy logic to exist in a private _destroy method, the same way _set contains component-specific logic and is called by set, so that destroy becomes part of the shared prototype as well. Anyway, the upshot is that at that point there would be two separate ways of attaching prototype methods.

Maybe I'll work that into this PR. Just about to make a start on #365 and #369

@Rich-Harris
Copy link
Member Author

Added the shared prototype thing — will shave off a few extra bytes. @Conduitry instead of having a dev helper lookup I thought it might be easier just to check for ${name}Dev in shared each time — that way we don't need to keep the lookup up to date if we add dev variants of other helpers.

@Conduitry
Copy link
Member

Conduitry commented Mar 16, 2017

Something that might be cool is to allow the -Dev versions of helpers call the regular versions, to avoid duplication and having to keep the two versions in sync. But I'm not sure how far we'd want to go in that direction. Keeping track of which functions call which other ones would be required for the non-shared version of the component, but sounds like it would be a lot of work for little benefit. Perhaps just put in a special case where xxxDev also brings in xxx?

Edit - Oh hm that's made a bit more complicated by lumping all the prototype methods into one export...

@Rich-Harris
Copy link
Member Author

Yeah, the same thought crossed my mind. I figured maybe we can see if it becomes a pain point (i.e. if the methods do indeed end up changing) and address it then, because I don't have any particularly good ideas for dealing with it right now

@Rich-Harris Rich-Harris merged commit 75abd0e into master Mar 16, 2017
@Rich-Harris Rich-Harris deleted the dev-helpers branch March 16, 2017 19:36
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.

2 participants