Skip to content

[worklets] (bikeshedding time!) import() name is potentially confusing... #374

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

Closed
bfgeek opened this issue Apr 5, 2017 · 18 comments
Closed

Comments

@bfgeek
Copy link
Contributor

bfgeek commented Apr 5, 2017

So the import function has been noted as potentially confusing.

Other names which have been discussed at some point are:
loadModule (@adamk)
distributeScript (@annevk)

For additional context the other way to invoke scripts will be via the script tag most likely, e.g.

<script worklet="paint" src="paint.js"></script>

Which is the same as

CSS.paintWorklet.import('paint.js');

From #373 originally.

@annevk
Copy link
Member

annevk commented Apr 6, 2017

Is there a dedicated issue for the script element extension? That will need some bikeshedding as well. In particular, it probably needs to use the type attribute to avoid running the script in older browsers.

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 6, 2017

Filed: #376 for designing that.

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 13, 2017

cc/ @nyaxt, @nhiroki for naming suggestions :)

@nhiroki
Copy link
Contributor

nhiroki commented Apr 13, 2017

Yeah, Worklet.import() and module’s import statement are so confusing when we discuss the design of implementation.

I feel Worklet.distributeScript() (or Worklet.distributeModule()) would be more descriptive of its behavior.

Other idea: Worklet.spawn()?

@annevk
Copy link
Member

annevk commented Apr 13, 2017

I like spawn, short and sweet.

cc @domenic

@domenic
Copy link
Contributor

domenic commented Apr 13, 2017

I don't have strong opinions. import() isn't that bad IMO. spawn() seems nice if it fits with the model. importInto() is another idea.

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 14, 2017

cc @surma halp.

@nyaxt
Copy link
Member

nyaxt commented Apr 14, 2017

I'm ok with import() or spawn()

@nhiroki
Copy link
Contributor

nhiroki commented Apr 14, 2017

Just in case, my previous comment is from implementer's pov. Probably web developers have different impressions, and I'm also ok with import() if they aren't confused by it :)

@surma
Copy link
Member

surma commented Apr 14, 2017

(Before I start: I could settle for spawn(), but I don’t think import() is confusing – on the contrary, its functionality is pretty similar, warranting a similar name. Edit: Or just create()?)

Okay, here goes: This has been bugging me for a while but I never said anything because I thought this was a done deal, but I guess now is my chance 😅

I always found it weird that theres some sort of magic global for each worklet that loads code. That code then proceeds to create an instance of something that – from there on in – would usually be referred to as “my XYZ Worklet”.

Taking some inspiration from workers, I’d find something like new CSS.PaintWorklet('myfile.js') much more intuitive. You‘d have a proper reference to hold on to for message passing and deregistering etc. Wondering what others think, if that’s semantically coherent and whether I just lost all credibility.

@surma
Copy link
Member

surma commented Apr 14, 2017

it probably needs to use the type attribute

What would the type look like, here? application/javascript+XYZWorklet?

@jakearchibald
Copy link
Contributor

Module scripts don't use a mime type, so this probably shouldn't. I think that discussion is happening in #376.

@jakearchibald
Copy link
Contributor

A constructor like @surma suggests makes sense if having an instance is useful.

In terms of a method, does it need to be more complicated than:

await CSS.paintWorklet.add('paint.js');

@annevk
Copy link
Member

annevk commented Apr 14, 2017

I think the reason there's no constructor is because it doesn't make sense to have more than one instance, so you'd need to keep global state to prevent multiple instances at which point you might as well just have it there. And the main reason to favor something like spawn over import is because the code can end up running in multiple globals (or is guaranteed to, in case of paint worklets).

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Worklets, and agreed to the following resolutions:

RESOLVED: import() to be renamed to addModule()
The full IRC log of that discussion
<eae> Topic: Worklets
<philipwalton> present+
<Rossen> present+
<skk> present+
<dbaron> Github topic: https://github.com/w3c/css-houdini-drafts/issues/374
<SimonSapin> Simon Sapin, Mozilla
<eae> iank_: There is the feedback from various people that the import method on worklets is confusing, similar to import statement in javascript which is somewhat similar but not really. Any preferences for another name? Looking for feedback.
<eae> iank_: Leading proposal is Worklet.spawn
<eae> Rossen: What is the history here? Feedback from people trying to use it?
<eae> iank_: Mostly feedback from annevk.
<eae> iank_: The main push back was that the script can end up running multiple global scopes which is a bit unique.
<eae> iank_: This was seen as confusing.
<eae> philipwalton: Definitively not import.
<eae> iank_: Any other preferences?
<eae> philipwalton: Could you clarify what it would be spawning if we go with spawn.
<eae> iank_: It is not really creating anything new, it loads a script into multiple global scopes. Not really spawning.
<eae> iank_: Another suggestion is loadModule. Primary push back there s that it looks similar to system.import which loads into the current global scope.
* astearns encabulate
<eae> philipwalton: Any disucssion around how this would interact with ES6 modules?
<eae> iank_: Would just work.
<eae> philipwalton: Other workers have the importScript function?
<eae> iank_: Yes and that would load into the current global scope.
<eae> iank_: dbaron, any preferences?
<eae> dbaron: not really
<eae> philipwalton: If the only difference is that it loads into multiple global scopes the name should reflect that.
<eae> dbaron: When you described what it does you used the word load....
<eae> iank_: Propose that we switch it to loadModule and see if there is any pushback for that specific name.
<eae> Rossen: Sounds good to me.
<eae> ???: I don't like spawn.
<eae> Sounds like ES6 load module but does somehting differently, my only concern.
<astearns> s/???/jack/
<eae> iank_: you need to have at least one global running. Your code will get immediately executed but if another global scope is spawn it'll load into that as well.
<astearns> s/Sounds like/flackr: Sounds like/
<eae> iank_: The paint specification for instance requires at least two global scopes at all times (unless under memory pressure).
<eae> rbyers: Perhaps register instead?
<eae> flackr: addModule?
<eae> iank_: I could live with addModule
<eae> iank_: Will update github issue to propose addModule and await feedback
<eae> Rossen: addModule is the final proposal for now?
<eae> philipwalton: I kind of like registerModule better but add is fine
<eae> Rossen: In summary, addModule is much preferred over import or spawn. Unless there are other proposal I suggest we resolve to rename it to addModule. Any objections?
<eae> RESOLVED: import() to be renamed to addModule()

@bfgeek
Copy link
Contributor Author

bfgeek commented Apr 19, 2017

@nhiroki FYI :)

@bfgeek bfgeek closed this as completed in b63a14e Apr 19, 2017
@nhiroki
Copy link
Contributor

nhiroki commented Apr 19, 2017

Chromium bug: https://crbug.com/713018

@bfgeek :)

@hoch
Copy link

hoch commented May 2, 2017

Oops. I really need to find a way to get notified this sort of change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants