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

[css-paint-api] Use JS export instead of registerPaint #564

Closed
rik opened this issue Jan 21, 2018 · 10 comments
Closed

[css-paint-api] Use JS export instead of registerPaint #564

rik opened this issue Jan 21, 2018 · 10 comments

Comments

@rik
Copy link

rik commented Jan 21, 2018

Instead of adding new syntax, could we reuse an existing one to register paint definitions?

I think export paintCtor as name is equivalent to registerPaint(name, paintCtor). This would even allow a simpler syntax export class foo{} instead of class foo{}; registerPaint('foo', foo).

For some reason, that might be undesirable but I thought I'd suggest it.

@surma
Copy link
Member

surma commented Jan 21, 2018

(It’s probably too late to change anything on this API, but I’ll try to give my thoughts anyways)

I see where you are coming from, but I don’t think your proposed API is actually desirable.
The way your API works is magic (implicitly registering an export — or all exports— as a paint class). What happens if I export a constant? Or a function? What if I want to share this module and only a subset of exports are actually paint classes?

The behavior that you describe can be built on top of the current API, so what we currently have is a low-level primitive that allows to build your more high-level approach.

const painters = await import('./your-painter-module.js');
for(const [name, painter] of Object.entries(painter))
  registerPaint(name, painter);

@surma
Copy link
Member

surma commented Jan 21, 2018

Edit: Turns out it’s not too late at all. Since Blink is the only engine actually implementing the API so far, things can still be changed without interop risk. Sorry if I discouraged you :)

@rik
Copy link
Author

rik commented Jan 21, 2018

What happens if I export a constant? Or a function?

Same thing that happens if you "registerPaint" a constant or function.

What if I want to share this module and only a subset of exports are actually paint classes?

If you never try to use those exports, then it does nothing. If they are used, then the errors in https://drafts.css-houdini.org/css-paint-api/#registering-custom-paint would trigger. I guess this would require changing the algorithms to trigger errors when there is a match between a paint function and a worklet export, rather than when at export time.

The behavior that you describe can be built on top of the current API, so what we currently have is a low-level primitive that allows to build your more high-level approach.

I'm not sure one is lower-level than the other. One is declarative, the other imperative.

@tabatkins
Copy link
Member

I don't understand how this would work. How would the browser know that the module in question is defining a custom paint class, and so should be registered as one? From the sample you've given, it just looks like an ordinary export.

(I agree with @surma in general - attaching extra magic to an export doesn't seem like a good direction. At least it's not one we've explored so far, and I'm not sure there's much, if any, precedence for this pattern among existing Node module usage.)

@rik
Copy link
Author

rik commented Jan 21, 2018

It would know from CSS.paintWorklet.addModule('my-custom-worklet.js'). Every export in my-custom-worklet.js would be registered as a custom paint class. The idea is that it would be an ordinary export to not introduce a new concept.

@plinss
Copy link
Member

plinss commented Jan 21, 2018

I agree with Tab and Surma, an interesting idea, but let's not.

  1. the whole point of Houdini is to not add more magic to the platform. Especially not adding declarative behavior to imperative code. This is imperative code for a reason.
  2. this overloads the concept of exporting from a module. What happens when there's a use for loading the same module elsewhere? There would then be a need to export things other than painters. Would we then need a new type of export?
  3. what about when we want to add new behaviors or classes to custom painters? Which is which?

We'd be giving up a lot of extensibility and future-proofing to save literally one line of code.

@surma
Copy link
Member

surma commented Jan 21, 2018

What happens if I export a constant? Or a function?

Same thing that happens if you "registerPaint" a constant or function.

Right. Currently, that throws. That means that all subsequent calls to registerPaint would never get executed. So we’d have to either live with that or catch them for you, but then you have no way of knowing if all your exports actually got processed. This also ties into the “reusing modules” point:
If I use a module somewhere outside a paint worklet, paint classes would just be vanilla classes. But if I export anything other than paint classes (consts, functions,...) re-using that module becomes problematic due to the reasons outlined above. It defeats the purpose of modularization (i.e. bundling data and functionality that belongs together).

That being said, I can see why your proposed way is desirable in terms of ergonomics. Seeing that you can build a declarative API on top of the imperative API with the 3 lines of code above and that the same is not true for the reverse case AFAICT, I think this is indeed the more low-level approach.

@rik
Copy link
Author

rik commented Jan 21, 2018

Let me show some examples with error handling to see if it makes sense. I've added an example from Animation worklets too.

// my-houdini-definitions.js
export const foo = 'bar';
export const baz = ''

export class rainbow {
  paint() {}
}

export class parallax {
  animate() {}
}
<script>
window.animationWorklet.addModule('my-houdini-definitions.js').then(() => {
    new WorkletAnimation('parallax', effects) // OK
    new WorkletAnimation('raindow', effects) // Throws
    new WorkletAnimation('foo', effects) // Throws
})

CSS.paintWorklet.addModule('my-houdini-definitions.js')
</script>

<style>
body {
    background: red;
    background: paint('rainbow'); /* Rainbow background */
}
body {
    background: red;
    background: paint('parallax'); /* Red background and logs an error in dev tools */
}
body {
    background: red;
    background: paint('foo'); /* Red background and logs an error in dev tools */
}

body {
    background: red;
    background: paint('unregistered') /* Red background and logs nothing because it might be registered later */
}
/* baz is exported but unused so nothing is logged for it either */
</style>

@css-meeting-bot
Copy link
Member

The Working Group just discussed JS export vs. registerBlah, and agreed to the following resolutions:

  • RESOLVED: Keep the current design
The full IRC log of that discussion <dael> Topic: JS export vs. registerBlah
<dael> github: https://github.com//issues/564
<dael> iank_: You can export things from a module should the engine be smart enough to crawl the export and pick off things that use paint.
<dael> iank_: Should we allow this in the spec?
<dael> surma: My reason was you can build tha ton the API we exposed.
<dael> surma: This seems more dev convenient, but I'm not sure how much they would experience the convenience since everyone is bundling.
<dael> surma: I think this is nice, but not worth the effort.
<dael> iank_: I think it's a bit of work for us. We don't have the onfirmation immediately exposed.
<dael> surma: Amy assumption that you can dynamically import and then tryand do exports, but if we don't have that you can't do it.
<dael> iank_: Do you think web dev coming to these APIs would expect...this is the first time an export could register
<dael> surma: We have used them, like the register call, which is not what I've seen in other APIs
<dael> dbaron: This may depend on how people persceve using them a couple years down the road. People who follow echnea script closely may have a better sense.
<dbaron> s/echnea script/ECMAScript/
<dael> fremy: You are supposed to be using import/export if you use modules, but I don't think there's an impression that the browser is pulling by default. I thought it was cool but if I have to write the funciton I'm not here.
<dael> majidvp: If you switch to exporting you don't know if it's paint or layout until you use module. BUt not when you register you register for a thing so you can verify.
<dael> iank_: Good point. Some oe fhte custom paint scripts we've seen people using it in the main winfow and the worklet. You can catch exceptions of registerPaint.
<dael> majidvp: Can you throw exception when you xport?
<dael> iank_: You can throw a global.
<dael> dbaron: Also search for where the function is. If it's export syntax I'm not sure how easy.
<dbaron> s/search for where the function is/you can search for the registerPaint name to find out what it does/
<dael> plinss: If we allow export we have to ad dmore clarity. You need another class to indicate this is a worklet. A bare export is really dangerious with extensible concerns.
<dael> iank_: It seems like an idea we'll keep for later if this becomes common.
<dael> Rossen: Sure.
<dael> flackr: Other way is at the point you add a module, naming the things you expect to be exported.
<dael> Rossen: Objections to keeping the current design?
<dael> RESOLVED: Keep the current design

@bfgeek
Copy link
Contributor

bfgeek commented Jun 21, 2018

Closing issue, as no changes needed.

@bfgeek bfgeek closed this as completed Jun 21, 2018
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

6 participants