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

subclassability #35

Closed
2 tasks done
dherman opened this issue Mar 12, 2015 · 9 comments
Closed
2 tasks done

subclassability #35

dherman opened this issue Mar 12, 2015 · 9 comments

Comments

@dherman
Copy link

dherman commented Mar 12, 2015

Following on this subthread, the Loader class should be subclassable. In particular:

  • the hooks should be inherited methods
  • each hook should be called on<hookname> or <hookname>Hook (discuss which)

What else?

/cc @matthewp @guybedford @johnjbarton

@johnjbarton
Copy link

For the Traceur loader I defined abstract hook methods in Loader and implemented them in subclasses. (The code is krufty with older abstractions, but this bit seems ok).

I'm unsure why you need the modifier on or Hook. Mostly these things are verbs, eg fetch fetches so it seems like it already has its best name. These aren't notification events so on does not seem like a good match.

@matthewp
Copy link

I would agree with @johnjbarton as far as the on prefix.

I understand the concern that they are primarily used internally but as discussed in the previous issue they can be useful externally (at least some of them) so I don't think it's so bad to name them as they are currently.

@dherman are they any performance considerations for too much inheritance? Is having 20 levels of subclassing faster/slower than having 20 wrapper functions? Or am I underestimating modern js vms and this is a silly question.

@dherman
Copy link
Author

dherman commented Mar 12, 2015

@johnjbarton Sorry, this is carry over from the conversation in #7:

I mean I could just not worry about it; there are only four hooks. But there are is one name collision (resolve) and one "conceptual" collision (load vs fetch). And still the purpose of these is as hooks, not as external methods. I think I'm more comfortable with a name convention.

@domenic
Copy link
Member

domenic commented Mar 12, 2015

I have a naive questions. Maybe it will help clarify things?

Loaders have a lot of methods. If my subclass overrides import, lookup, uninstall, cancel, etc.---will those be called by the implementation sometimes? Or are they only called by author code?

@matthewp
Copy link

Yes, you absolutely should be able to override those. I see no difference between the hooks and those methods. import is one that is useful to override in some advanced scenarios.

@dherman
Copy link
Author

dherman commented Mar 12, 2015

@domenic The only methods that are triggered by the internal semantics of the loading process are the hooks. Those other methods (import, lookup, uninstall, cancel etc) are only triggered by user code.

@matthewp I believe both deep inheritance and deep wrapping can be expensive but I don't know enough about modern engines to compare them intelligently. I'm not super worried about this since I expect loader configuration to be a pretty top-level thing (comparable to, e.g., installing polyfills with a top-level script) so I don't expect to see lots of loader extension and configuration sprinkled throughout codebases.

@johnjbarton
Copy link

All of these hooks should be called on the order of once per module. Since the cost of processing each module is very large compared to any method invocation sequence, I vote for the solution that is the clearest for developers.

@caridy
Copy link
Contributor

caridy commented Aug 21, 2015

Solved by #65

@caridy
Copy link
Contributor

caridy commented Aug 21, 2015

subclassing is now supported via:

class MyLoader extends Reflect.Loader {}

hooks are now defined thru symbols:

System.loader[Reflect.Loader.fetch] = function (key) {
   // implement your custom fetch on the default loader
};

// or
let myLoader = new MyLoader();
myLoader[Reflect.Loader.fetch] = function (key) {
   // implement fetch on your custom loader instance
};

@caridy caridy closed this as completed Aug 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants