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

Delete old cljs instrumentation code #1013

Closed
wants to merge 1 commit into from

Conversation

ikitommi
Copy link
Member

@ikitommi ikitommi commented Feb 29, 2024

test break, see #890

@dvingo
Copy link
Contributor

dvingo commented Mar 5, 2024

There is one use-case in favor of keeping this around (but maybe renaming the namespaces?), which is for the use-case where you would want to run instrumented code with advanced compilation (and during development would still use the new namespacemalli.instrument)

Just came up on slack:
https://clojurians.slack.com/archives/CLDK6MFMK/p1709638318412439

@ikitommi
Copy link
Member Author

ikitommi commented Mar 6, 2024

thanks, was about to ping you if this can be removed. But this doesn't work with latest versions of everything, as mentioned in Slack. Options:

  1. remove it
  2. fix it and maybe rename?

WDYT? would you have time to do the 2?

@ingesolvoll
Copy link
Contributor

@ikitommi @dvingo I'm in the process of updating this code to use it for instrumenting my production build during E2E testing. But I'm perfectly fine with inlining the legacy malli code in my codebase, doesn't seem like there's much reason to keep it around here.

@ingesolvoll
Copy link
Contributor

That said, looks like the only thing not working is the goog.mixin thing, which has a trivial fix.

@ingesolvoll
Copy link
Contributor

#1016 fixes the goog/mixin issue, and code works for me with that fix

@ikitommi
Copy link
Member Author

closing this.

@ikitommi ikitommi closed this Mar 23, 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

Successfully merging this pull request may close these issues.

3 participants