babashka / sci.configs

A collection of ready to be used SCI configs
https://babashka.org/sci.configs/
Other
19 stars 17 forks source link

Re frame alpha config #43

Closed kimo-k closed 7 months ago

kimo-k commented 7 months ago

Re-frame has a new re-frame.alpha ns since 1.4.0. Upgraded re-frame and integrated the alpha ns.

borkdude commented 7 months ago

@kimo-k I don't understand adding .alpha in namespace names. Does this mean it shall be removed later once re-frame decides that namespace becomes beta or production-worthy? And then this will be removed from sci.configs which will mean breaking changes for everyone who uses it? Isn't a main idea of Clojure to never have breaking changes? I'd rather not disappoint my users by later removing the alpha namespace.

kimo-k commented 7 months ago

Hey, I hadn't thought of that perspective. From our end, I can say alpha is not going to be removed any time soon.

Do you think this would cause breakage for people who don't import re-frame.alpha in their app code? If not, a case might be made that anyone who imports an alpha accepts tacitly that it could break on upgrade, or really, any time.

borkdude commented 7 months ago

@kimo-k Once your publish, you're stuck with it because users rely on it. I don't want to break users. And with that in mind: with this PR, the alpha namespace becomes part of the re-frame bundle of e.g. scittle, there is no way to separate them out. Two options:

kimo-k commented 7 months ago

Once your publish, you're stuck with it because users rely on it

This seems hard to agree with. To rely on an alpha - isn't this a contradiction in terms? That said, you made a high-level analysis that I may not fully understand. Also with the bundling options and artifact size, I'm not sure what the ramifications are. Are you sure the impact would be high enough to matter?

For my part, I'll try to answer this more directly:

And then this will be removed from sci.configs which will mean breaking changes for everyone who uses it? Isn't a main idea of Clojure to never have breaking changes?

Yes, we would remove alpha eventually, or stub it out, if that's more future-proof. But AFAICT, alpha namespaces constitute the exception which makes this principle practical to uphold. A place where "breaking changes" are justified by the use-case. In our case, we intend re-frame.alpha to be something users can try out, so they can give feedback. We need a SCI config so that we can demonstrate the alpha features. That said, I'm not a tooling expert, so maybe there's a better approach that I'm missing.

Put the re-frame.alpha SCI config stuff in a separate optional namespace such that scittle can make it an optional asset, such that it won't affect the normal re-frame plugin size

I'll have to do some research & figure out how this would look in practice.

borkdude commented 7 months ago

Many people rely on clojure.spec.alpha. The Clojure team would never remove this since many Clojure programs would fall over. I'd say just don't make the same mistake.

kimo-k commented 7 months ago

Does this mean it shall be removed later once re-frame decides that namespace becomes beta or production-worthy?

Also, in case it matters: once any features pass alpha, they'll merge into re-frame.core. We won't ever provide user-facing API in re-frame.flow, for instance. re-frame.core is a monolith, for better or worse.

borkdude commented 7 months ago

So what I'm saying: if the namespace is there, it will always be there, but it should imo be optional as to not bloat the size of the "main" stuff.

This is why we can make the re-frame.alpha stuff optional but placing it in a different namespace. You can see how that is done for reagent.dom.server here:

https://github.com/babashka/sci.configs/blob/main/src/sci/configs/reagent/reagent_dom_server.cljs

It's pretty straightforward.

kimo-k commented 7 months ago

Okay, gave it a try. I assume it's still okay to use sci/copy-ns, even though reagent-dom-server does something else.

borkdude commented 7 months ago

@kimo-k If you try running the playground, then you'll see that it doesn't work yet over there since the configuration must still be merged in here:

https://github.com/babashka/sci.configs/blob/91c8eb918c80edaa0c1440070023a05b662538aa/playground/src/playground.cljs#L60

Can you run this locally and verify that it works after doing so? The playground will be automatically updated after merge here: https://babashka.org/sci.configs/

borkdude commented 7 months ago

Sorry for closing, accidentally hit the button

kimo-k commented 7 months ago

Fixed & tested.

Screenshot_2023-11-22_14-39-04

borkdude commented 7 months ago

Awesome, thanks!