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

Re frame alpha config #43

Merged
merged 5 commits into from
Nov 22, 2023
Merged

Conversation

kimo-k
Copy link
Contributor

@kimo-k kimo-k commented Nov 21, 2023

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

@borkdude
Copy link
Collaborator

@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
Copy link
Contributor Author

kimo-k commented Nov 22, 2023

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
Copy link
Collaborator

borkdude commented Nov 22, 2023

@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:

  • We can wait until the alpha stuff comes out of alpha
  • 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

@kimo-k
Copy link
Contributor Author

kimo-k commented Nov 22, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

kimo-k commented Nov 22, 2023

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
Copy link
Collaborator

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.

Re-frame has this new alpha namespace since v1.4.0
@kimo-k kimo-k force-pushed the re-frame-alpha-config branch 4 times, most recently from 8793147 to 4ecc19d Compare November 22, 2023 13:24
@kimo-k
Copy link
Contributor Author

kimo-k commented Nov 22, 2023

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

@kimo-k kimo-k force-pushed the re-frame-alpha-config branch from 4ecc19d to 5299092 Compare November 22, 2023 13:27
@borkdude
Copy link
Collaborator

@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:

#'sci.configs.re-frame.re-frame/config

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 borkdude closed this Nov 22, 2023
@borkdude borkdude reopened this Nov 22, 2023
@borkdude
Copy link
Collaborator

Sorry for closing, accidentally hit the button

@borkdude borkdude merged commit 856b15b into babashka:main Nov 22, 2023
3 checks passed
@kimo-k
Copy link
Contributor Author

kimo-k commented Nov 22, 2023

Fixed & tested.

Screenshot_2023-11-22_14-39-04

@borkdude
Copy link
Collaborator

Awesome, thanks!

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