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

Incorporate missing facades used by Indigo #469

Open
davesmith00000 opened this issue Aug 10, 2021 · 11 comments
Open

Incorporate missing facades used by Indigo #469

davesmith00000 opened this issue Aug 10, 2021 · 11 comments

Comments

@davesmith00000
Copy link
Contributor

This is a slightly lazy issue, apologies for that, but I caught the new maintainer announcement and thought I'd get in quick! :-)

Indigo uses Scala-js-dom a lot as you can imagine, but in a very few specific cases I've had to write my own facades or extensions to existing facades where functionality is missing, particularly for WebGL 2.0 support.

They all live here, bear in mind I don't believe WeakMap is in use, and they aren't supposed to be complete interfaces they only add what I need, but if it helps:

https://github.com/PurpleKingdomGames/indigo/tree/master/indigo/indigo/src/main/scala/indigo/facades

I don't know how much time I have but I could try and wrap some of these up into a PR if it helps.

@sjrd
Copy link
Member

sjrd commented Aug 10, 2021

WeakMap is standardized in ECMAScript, so it should be added directly in the Scala.js standard library, instead of here.

@armanbilge
Copy link
Member

Thanks for this! If you have a chance, would be awesome if you can make a PR even if it's only for a few of the missing things, anything helps :)

@davesmith00000
Copy link
Contributor Author

Hi, I came back to have a look at this.

The interfaces I've got (and I haven't checked if they've been added since I wrote them yet) are:

  • FontFace
  • TextMetrics
  • FullScreenElement
  • IndigoCanvasRenderingContext2D which just adds var direction: String = js.native to CanvasRenderingContext2D
  • WebGL2RenderingContext which extends WebGLRenderingContext

The last one is the biggest, but all of them are partials that just add the features I needed, and are not scaladoc'd yet (but I can have a go).

It looks there are two groups of facades to move over, the first four small an uncontentious ones that I may do as a separate PR each (assuming I find they are still missing).

The more dubious one I was going to move over was the WebGL 2.0 stuff. I had a quick scout around the project though and noticed the WebGL 1.0 discussion:

#352
#370

Technically Safari hasn't adopted WebGL 2.0 yet - it is imminent I believe (https://caniuse.com/?search=webgl%202) - but they are way behind. Should the WebGL 2.0 interfaces be in the experimental area do you think?

@davesmith00000
Copy link
Contributor Author

Went in to add the following to CanvasRenderingContext2D :

  /**
   * Possible values: ltr, rtl, inherit
   *
   * MDN
   */
  var direction: String = js.native

But according to MDN this is experimental too.

What do we do in this case?

@armanbilge
Copy link
Member

@davesmith00000 Thanks for looking into this!

It looks there are two groups of facades to move over, the first four small an uncontentious ones that I may do as a separate PR each (assuming I find they are still missing).

That would be awesome, thanks!!

Regarding experimental stuff, there's also the separate issue of how well-supported it is across browsers. E.g., an API could be experimental but still supported by all major browsers. On the other hand, it might not be experimental but that doesn't matter if browsers don't support it 🙃 In any case should still have a formal strategy how to determine these cases going forward, cc @japgolly.

Looking specifically at CanvasRenderingContext2D.direction, it falls into the unfortunate category of being both experimental and also completely lacking support in Firefox.

@davesmith00000
Copy link
Contributor Author

Ok, so philosophically the question is:

Does this library represent the full specification and require the developers to know what they can and can't use (which is the case if you are a normal JS developer), or only a subset of the features that are known to be broadly supported?

Which I believe is related to what #514 is about. I'm completely fine with either, seems like you're suggesting it's a subset? I think that's the more difficult path to follow.

Personally, I'd expected the library to attempt to represent the whole specification without fear or favour, but maybe that's just too much work or undesirable for some reason? Nonetheless, if thats the case then the ones to add from my list are (I think - assuming we don't care about IE?):

  • FontFace
  • TextMetrics
  • FullScreenElement
  • WebGL2RenderingContext

I don't believe any of that is experimental, but then again, Safari won't do WebGL 2 and support for TextMetrics is patchy... Bah! 😄

@armanbilge
Copy link
Member

Personally, I'd expected the library to attempt to represent the whole specification without fear or favour, but maybe that's just too much work or undesirable for some reason?

Personally I'd lean this way myself, especially via a scheme like #487. But historically speaking, I'm not 100% what this library's mission is. The use of an experimental package and frequent references to www.caniuse.com in PRs suggests that it is not that.

@sjrd
Copy link
Member

sjrd commented Aug 28, 2021

I would suggest leaving experimental in the past, and represent instead the full spec, independently of browser support. Otherwise, there's never a good time nor a good way to have things "graduate" out of experimental anyway.

@davesmith00000
Copy link
Contributor Author

Ok so... I've read the updates on issues #514 and #487 there's a bit of a dependancy chain here I think.

If you go ahead and do code generation then this ticket (and probably many others...?) can be closed as redundant.

Assuming you don't do that (yet?), in order to do this ticket we'd ideally need #370 closed/merged and the experimental package deprecated/merge to avoid confusion.

Then I could merge over the missing stuff, and then you'll need to draft some exciting comms for the next round of release notes explaining all this. 😄

I'll hold off until I hear back. Equally happy for someone else to beat me to it. 😉

@armanbilge
Copy link
Member

@davesmith00000 thanks for following along!

At the moment, a code generation scheme is still a bit mythical, so I wouldn't worry about that 😆 (although, if you feel inclined please add a comment showing your support in that discussion, if we can get a signal that the community is happy with the positives/negatives of an auto-generated codebase that is extremely helpful).

But you are right, I think the next big step in scala-js-dom is cleanups etc. occurring in #458 and possibly some follow-up PRs. However, as long as you follow the (currently draft) contributing guidelines proposed in #523 I think we should probably be able to merge in your work fairly easily despite all the shuffling around. Could be wrong though so maybe the safer strategy is to wait 😅

@tsnee
Copy link

tsnee commented Mar 10, 2024

I just opened #838. It goes a bit beyond the original request and is my attempt to add complete WebGL 2 support.

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

4 participants