Allow a prop as to change the tag at runtime

davesnx / styled-ppx

Type-safe styled components for ReScript, Melange and native with type-safe CSS

https://styled-ppx.vercel.app

BSD 2-Clause "Simplified" License

399 stars 32 forks source link

Allow a prop as to change the tag at runtime #436

Closed zakybilfagih closed 5 months ago

zakybilfagih commented 5 months ago

My attempt to add as props. #296

For ReScript, I thought of using \"as" instead of as_, maybe we can handle this conditionally?

vercel[bot] commented 5 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

zakybilfagih commented 5 months ago

In order to have it to work with both ReScript and Reason, the best is to use the same mechanism as https://github.com/davesnx/styled-ppx/blob/main/packages/ppx/src/platform_attributes.re, so in ReScript can keep it as "as" I believe.

I can't seem to make \"as" work when generating for ReScript (using illegal identifier names). Since the output should be valid OCaml syntax (?)

Using as_ works fine though.

zakybilfagih commented 5 months ago

@davesnx Managed to get it to work :) But some of PPX tests for ReScript is broken (calling rescript convert <..>.ml). Since bsc is outputting invalid OCaml syntax (?)

davesnx commented 5 months ago

I have found those issues but they are flaky, I added a few "fixes" like this one https://github.com/davesnx/styled-ppx/pull/436/files#diff-d8703a331a10e5671566f821101c2934d0de98e4464942d485fdd5f03db425b2R4 but still they appear from time to time. Try to cat into the res file to see if it's really a syntax issue (and mitigate it or keep the test as it is now)

Once this is fixed, I will merge this, looks great.

zakybilfagih commented 5 months ago

I cat'ed the outputted ml file from bsc, and confirmed that the error is caused by using as as a record field and doing record access (props.as).

We can maybe replace those with sed, or is it too hacky?

davesnx commented 5 months ago

Right, in ReScript we use record field access for props (prop.bla) but in Reason we use blaGet(prop). I wonder if we need to apply a small patch for "as" in the code gen and do: https://rescript-lang.org/try?version=v11.0.1&code=ALBWGcA8GEHsDsBmBLA5gCgN4DcCmAncZBALgAIAWAGjIFtYATXcgIgEMBXAF1lra+QBjFgF8AlACgJXAJ4AHXGXy5BsfAwD6Ad2RcAFhrbgyAXjKYAOu3Aty4LvmTxUIqQBtcXMrkhtacj3JlVXVtXQMjU3MrI1syawZEUSkAKXAAOjdYDB8-ANx0mJtJIA

zakybilfagih commented 5 months ago

I've tested the e2e for both v9 and v10 ReScript and the PPX is working correctly (using as).

The problem is I think with the ppx cram test.

Call bsc with the ppx and input.res file -> this outputs .ml file
Since the AST contains a record field as (which the ReScript syntax happily supports) it spits out a record field as in OCaml syntax which is not supported
When trying to convert back the .ml file using rescript convert, it errors out because record field as is not valid syntax

davesnx commented 5 months ago

I see, then adding a sed (and a comment explaining this) is really good then

zakybilfagih commented 5 months ago

The CI for ReScript v9 failed with this message, I've ran the test locally and is working fine 🤔

  We've found a bug for you!
  /home/runner/work/styled-ppx/styled-ppx/_build/default/packages/css/rescript/Kloth.ml:37:59-63

  35 │   let get = String.get
  36 │   let length = Js.String.length
  37 │   let startsWith affix str = Js.String.startsWith ~prefix:affix str
  38 │ end
  39 │

  The function applied to this argument has type Js.String.t -> bool
This argument cannot be applied with label ~prefix

davesnx commented 5 months ago

Fixed in main. Rebase/merge fixes the issue. I could repro locally, can you make sure you have melange.3.0.0 installed?

zakybilfagih commented 5 months ago

Ooh yeah I forgot to sync with upstream. Removed the pin and upgraded the deps!

zakybilfagih commented 5 months ago

Fixed the cram tests!