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

Qwik QRL #498

Closed
brandonstubbs opened this issue Mar 30, 2024 · 14 comments
Closed

Qwik QRL #498

brandonstubbs opened this issue Mar 30, 2024 · 14 comments

Comments

@brandonstubbs
Copy link
Contributor

In the Qwik Framework, they have a concept of QRL.

I have the following button:

(ns app
  (:require ["@builder.io/qwik" :refer [component$ useSignal]]
            ["./app.css"]))

(defn -App []
  (let [count (useSignal 0)]
    #jsx [:button {:onClick$ #(set! (.-value count) (inc (.-value count)))}
          "count is " (.-value count)]))

(def App (component$ -App))

This is the output

<button onClick$={(function () {return count1.value = (count1.value + 1);})}>count is {count1.value}</button>

This would be the ideal output:

<button onClick$={() => { return count1.value = (count1.value + 1);}}>count is {count1.value}</button>

Qwiks $ optimiser needs serializable values.

@borkdude
Copy link
Member

Can you be more specific on what is the difference in output? Is that only the arrow function vs function function?

@brandonstubbs
Copy link
Contributor Author

Yeah seems it works better with the arrow function clojure output

@borkdude
Copy link
Member

I don't see how function is less serialisable than () => {}

@brandonstubbs
Copy link
Contributor Author

This is the error output:
image

Maybe more an issue their side

@borkdude
Copy link
Member

So I guess we could use a syntax in squint to produce "arrow" functions instead, e.g. (fn ^:whatever []), I'm not sure. Any ideas?
Also, can you try out if changing that function to () => {} in the output really fixes the issue?

@brandonstubbs
Copy link
Contributor Author

Yeah changing the output fixes the issue.

Is there a reason the function is preferred over arrow functions? I wonder if maybe this would be better to be a squint compiler flag to emit all functions as arrow functions? Or at least functions in JSX as arrow functions.

@borkdude
Copy link
Member

I some cases function is necessary since () doesn't support this

@brandonstubbs
Copy link
Contributor Author

I think let's park this for now. I am not familiar with the framework. I was just getting the vite templates to work. I will open an issue with them first to see why the function way doesn't work, as (def App (component$ -App)) this part is also a $ optimisation and seems to work.

@martinklepsch
Copy link
Contributor

Can function be serialized less well than arrow fns? This seems like an odd requirement on Qwiks side.

Could be useful to provide some low level syntax helpers in squint to emit the respective syntax, maybe?

@brandonstubbs
Copy link
Contributor Author

brandonstubbs commented Mar 30, 2024

So it seems you can wrap the function in their $ function:

(ns app
  (:require ["@builder.io/qwik" :refer [component$ useSignal $]]
            ["./app.css"]))

(defn -App []
  (let [count (useSignal 0)]
    #jsx [:button {:onClick$ ($ #(set! (.-value count) (inc count)))}
          "count is " (.-value count)]))

(def App (component$ -App))

This compiles to:

<button onClick$={$((function () { return count1.value = (count1 + 1);}))}>count is {count1.value}</button>

If the extra paren wasn't added around the function this would work. It should look like this instead:

<button onClick$={$(function () { return count1.value = (count1 + 1);})}>count is {count1.value}</button>
//                 ^

@egasimus
Copy link

egasimus commented Mar 30, 2024

I some cases function is necessary since () doesn't support this

Can function be serialized less well than arrow fns?

a squint compiler flag to emit all functions as arrow functions?

Arrow functions are not just syntax sugar, they have different behavior. Though the exact description of the main difference eludes me right now (something something scoping?), here's a quick example:

$ node

> (function(){}).prototype
{}

> (()=>{}).prototype
undefined

> new (function(){})
{}

> new (()=>{})
Uncaught TypeError: (intermediate value) is not a constructor

Another difference is hoisting.

Not being able to specify the kind of function in Squint is kind of a non-starter for me.

A ^:something would be very much welcome; but since arrow functions are also a shorthand, IMHO (fn ...) should emit arrow functions by default and the longer form (fn ^:something) should be used for full functions.

@borkdude
Copy link
Member

IMHO (fn ...) should emit arrow functions by default

This isn't done because of compatibility with existing CLJS code

@martinklepsch
Copy link
Contributor

I created an issue but also realized that maybe we should just rename this one, then all context is in one place.

@borkdude
Copy link
Member

borkdude commented Apr 5, 2024

#499 is fixed, I think this should work now

@borkdude borkdude closed this as completed Apr 5, 2024
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

No branches or pull requests

4 participants