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

Introduce tidy-codegen and Deku.DOM.Indexed #101

Merged
merged 31 commits into from
Aug 19, 2023
Merged

Conversation

jterbraak
Copy link
Collaborator

I really like what you have done with the framework but the one thing i miss from Halogen is the way it defined elements using row types. The current implementation also seems to increase startup-time of the compilation by about 2-3 seconds. This pull-request aims to add an alternate set of modules for element generation that is still compatible with the primary DOM-elements.

This will also replace the existing codegen from the python scripts to a project written in Purescript itself. Additionally it sources its data mostly from the available spec instead of scraping MDN.

@jterbraak jterbraak changed the title Introduce tidy-codegen and Deku.Indexed.DOM Introduce tidy-codegen and Deku.DOM.Indexed Jul 24, 2023
@jterbraak jterbraak marked this pull request as draft July 25, 2023 08:10
@mikesol
Copy link
Owner

mikesol commented Jul 26, 2023

Oh wow, this is neat!
Let me know when you'd like some review and I can start reading through the PR and leaving comments.

@mikesol
Copy link
Owner

mikesol commented Jul 27, 2023

Also, one thing to note is that a lot of work is going into deku for a 1.0, starting with the backend optimizer, working its way through purescript-hyrule and purescript-bolson and finally hitting this repo. There are many PRs preparing this, so given how extensive this PR is, I thought I'd give you the heads up just so that you don't hit merge conflict hell with it. We're still at least a week off (if not more) from merging any of that stuff. Here are some links:

The basic idea is to make Deku (and a few other projects) as fast as Astro by doing aggressive inlining when possible. This only works, though, if several things change in concert.

If you're interested in participating in this effort, a few of us are hacking away at it & I'd be happy to give you more info.

@jterbraak
Copy link
Collaborator Author

The branch is working again and the tests are passing. There is still an issue with a missing unset function in deku-dom-indexed but I want to merge fantasy-land first as the new attribute representations makes the implementation easier.
I didn't want to force deku-dom-indexed on every user so I also split the package using new the spago version and added a spago.yaml to every sub-package. I also went ahead and split off the CSS module.

How would you like to handle #100? In this implementation they both would be DOM.Type. I tried to minimize escaping where it was not necessary but I don't know why Xtype was named like that in the original version.

Another issue is the namespace support. There is some code in

const svgTags = new Set([
that hacks in SVG support for elements. I would like to patch this by extending elementify and friends but the original DOM modules only have one function per tag name. Elements like title are in both SVG and HTML so there should be a way to select either one. My preferred way would be to split the giant DOM module into SVG and HTML like i've done for DOM.Indexed.

@jterbraak jterbraak marked this pull request as ready for review July 31, 2023 11:18
Copy link
Owner

@mikesol mikesol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

Merging this and fantasy-land will be delicate, so we should coordinate the two over the next couple weeks to make sure that process goes smoothly. Also, fantasy-land may get the axe depending on the community's response. I still need to socialize it more & I'll know by early next week if it makes the cut.

Deku currently has a small user base, so in my opinion it only makes sense to have one representation of the DOM. I'm not particularly attached to the original one, so if you feel that the indexed one is more ergonomic, we should go for that.

The Xtype is just a random hack because at some point someone didn't want the word Type used & the framework was in its early stages so we ran with it. It'd be better to follow a consistent pattern & document that.

So the action points I'd recommend are:

  1. Nix the current DOM representation unless you believe it covers a different use case than the indexed representation.
  2. Add namespacing for SVG & potentially other namespaces, as you recommended in your comment.
  3. Add tests. If you have already that's great, the PR is big so I may have missed them. I use JSDOM in the current test suite & you can tack stuff onto that or make a separate suite if you decide to keep both representations and don't want to mix together the tests.

Thanks for this great work & I'm looking forward to getting it into the framework once it's ready!

Joost ter Braak added 2 commits August 16, 2023 14:59
fixed examples and tests
added _unset function, this will create an unsetter for an attribute
constructor
added EventEffect type alias to make signatures for interfaces smaller
@jterbraak
Copy link
Collaborator Author

The old DOM modules are removed as requested. I fixed up the tests to use the indexed version and they pass. elementify has been patched now supports proper namespaces.

One more thing about namespaces:
I opted to put elements, attributes and values into the same module/namespace. Obviously this creates some conflicts between, for example, the element title and the attribute. So I prefixed attributes with a _. This way all HTML/SVG/MathML related values still fit into one module.
If you think this convention is ugly or confusing I can still split them up like Halogen does. I.e. a HTML module for elements and Attributes for the rest.

When rewriting applicatons these regexes helped me in VS Code:
D\.([A-Z])(\w+)\s+!?:= replace to D._\l$1$2_

@jterbraak jterbraak requested a review from mikesol August 16, 2023 13:29
Copy link
Owner

@mikesol mikesol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great! Definitely an improvement.
How would you feel about doing codegen for stuff like click_ etc as well? It's a bit dicier because a lot of those listeners have additional transformations (like slider_) but it'd be more comprehensive and consistent. But if that's too much to pick off on this PR we can just do a followup, and if it's not desirable we can table the idea. I only bring it up because, as we're already doing a major reorg here, it's good to get everything done in one fell swoop if it's not too much additional burden.

Great work!

@mikesol
Copy link
Owner

mikesol commented Aug 16, 2023

If you think this convention is ugly or confusing I can still split them up like Halogen does. I.e. a HTML module for elements and Attributes for the rest.

I don't think it's ugly nor confusing, but in the absence of any other factors, I'd opt for the Halogen one because it's something that is likely familiar to folks.

However, vision trumps prior art, so if there's a reason you want them all in one place & feel somewhat strongly about it then let's go with that.

@jterbraak
Copy link
Collaborator Author

What do you mean with generating stuff like click_? I could overload _onClick and add cases for Effect Unit and Cb. I wanted to avoid that because I find that typeclasses and overloads are bad for disoverability in the editor. I still kinda want to axe the whole Keyword system.

@mikesol
Copy link
Owner

mikesol commented Aug 16, 2023

I still kinda want to axe the whole Keyword system.

What do you mean by the Keyword system. Stuff like D._id_?

@jterbraak
Copy link
Collaborator Author

For _type i generate all valid cases. For example Keyword "number". An overload of _type then allows only valid keywords and String. So you can write:

D.input [ D._type_ D.__number ] ...

@jterbraak
Copy link
Collaborator Author

Also after writing the last comment I'm coming around to moving attributes to another module.

@mikesol
Copy link
Owner

mikesol commented Aug 16, 2023

For example Keyword "number".

If you axed this, what would you replace it with?

@jterbraak
Copy link
Collaborator Author

jterbraak commented Aug 16, 2023

Well the old implementation left them stringly typed so we could go back to that or generate an actual concrete type with a String escape.
ala:

data InputType
    = InputTypeNumber
    | InputTypeText
    ...
    | InputType String

Kinda like what Halogen does :). Although they use handwritten code to convert from the Web.HTML modules and I'm not about to do that.

@mikesol
Copy link
Owner

mikesol commented Aug 16, 2023

Got it. So my general philosophy is that if there's gonna be an ADT for one enum in the Dom spec for attributes (in this case an input's type), we should have ADTs for every enum in the Dom spec for attributes. This is the rabbit hole I went down with purescript webgpu, which is completely free of string-typing. But it only makes sense if the spec is ironclad, not meant to be broken, and explodes in unpredictable ways when you use anything other than spec conformant values (that's WebGPU in a nutshell). For more permissive and lenient stuff, I'm a fan of strings.

@jterbraak
Copy link
Collaborator Author

So the input type was an example. There are loads of keywords for autocomplete, sandbox etc. But the "Spec" does not always specify whether the keywords are exclusive. In HTML anything goes. So most of the time we have to allow String. I now tend towards applying the keywords beforehand:

type = ...
type_ = ...
typeNumber = type_ "number"
typeText = type_ "text"
typeDate = type_ "date"

So when you type Attr.type in the editor all the options get listed. And the escape is still there as type.

@mikesol
Copy link
Owner

mikesol commented Aug 16, 2023

Would it make sense, then, to do this for all of the enums (autocomplete, type, sandbox, etc) - ship the standard ones and always provide a backdoor for custom ones?

@jterbraak
Copy link
Collaborator Author

Yeah, that's my plan. I'll get this and the separate Attribute modules done in the morning.

Still not sure what you mean with the click_ changes?

@mikesol
Copy link
Owner

mikesol commented Aug 16, 2023

I mean something like:

import Deku.DOM as D
import Deku.Attr as DA
import Deku.Listeners as DL

foo = D.button [DA.id_ "foo", DL.click_ (alert "hello") ] []

That sort of thing. Basically that we'd have a namespace for the listeners as well that is as exhaustive as the attributes. Which is a bit harder because there's more diversity there, for example certain listeners are "smart" enough to listen to numbers (like slider), keys (like keyUp) etc.

- removed "Keyword" and replaced it with more variants of the base attribute
- dropped the "on" prefix for event handlers and added "Combinators"
@jterbraak
Copy link
Collaborator Author

jterbraak commented Aug 18, 2023

With the changes it's now possible to write

import Deku.DOM as D
import Deku.DOM.Attributes as DA
import Deku.DOM.Listeners as DL

foo = D.button [DA.id_ "foo", DL.click_ \_ -> alert "hello" ] []

I might be possible to add more variants for every event but I don't really see the point of making it any simpler. I also moved the old Listeners module into Combinators and reexported it from every Listeners module. That enables code like:

import Deku.DOM as D
import Deku.DOM.Attributes as DA
import Deku.DOM.Listeners as DL

foo = DL.div_
  [ DL.input [ DA.xtypeNumber, DL.valueOn DL.input alert ] []
  , DL.input [ DA.xtypeRange, DL.numberOn  DL.input $ show >>> alert ] []
  , DL.input [ DA.xtypeCheckbox, DL.checkedOn DL.input $ show >>> alert ] []
  ]

With the change to the modules type, in and class have to be escaped. This is done in Common.escape, which generates xtype, xin and klass.

@mikesol
Copy link
Owner

mikesol commented Aug 18, 2023

Awesome! Lemme know when it's ok to merge & I'll merge it!

@jterbraak
Copy link
Collaborator Author

Still got some ideas but I'm about done with this part. You can merge it.

@mikesol mikesol merged commit 9b23938 into mikesol:main Aug 19, 2023
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