cljs instrumentation DX - clear metadata schemas before collection #745
Conversation
During development with instrumentation enabled function schemas declared with metadata were never deregistered once they were collected. Thus if a developer removed a schema on a function it would always be instrumented. This commit adds a flag on metadata schemas to distinguish them from => style schemas and then removes them before running collection again in malli.dev.cljs/start!
| @@ -17,6 +18,8 @@ | |||
| (defn start!* [env options] | |||
| `(do | |||
| ;; register all function schemas and instrument them based on the options | |||
| ;; first clear out all metadata schemas to support dev-time removal of metadata schemas on functions - they should not be instrumented | |||
| ~(m/-deregister-metadata-function-schemas! :cljs) | |||
There was a problem hiding this comment.
Should this be called on stop! as well?
There was a problem hiding this comment.
I think it is not needed on stop! because there are two separate concerns:
- collecting the functions that have schemas and storing them in a hashmap in an atom
- starting and stopping instrumentation
the -deregister helper is to deal with ensuring those functions stored in the atom are up to date with the code, whereas stop!ing is only concerned with replacing the instrumented function with its original function.
I could see some weird interaction happening with hot reloading where you both remove the schema and change the function implementation, and then call stop! which may replace the updated function with the original function.
But this would only affect one reloaded app instance, you would just have to hot-reload again and the newest function will be in place.
I tried out running stop! after removing metadata and things work as expected, but of course if you run into something strange let me know.
Do you have video demo or a reproduction repo to look at? The video I posted above is doing what you described, so a repo would help. |
|
@dvingo Sorry, I suspect this is coming from the way our re-frame variant caches subscriptions and events, and probably not relevant to your change. Will report back if I find any evidence pointing elsewhere. |
|
Just say when all good and I'll merge this. |
|
I am good with this being merged. If there are further complications another issue can be opened. |
|
Merged it is then, big thanks for the super quick fix. |
During development with instrumentation enabled function schemas declared
with metadata were never deregistered once they were collected.
Thus if a developer removed a schema on a function it would always be instrumented.
This commit adds a flag on metadata schemas to distinguish them from => style schemas
and then removes them before running collection again in malli.dev.cljs/start!
This resolves
#744
demo of metadata schema being added and removed (this is in the sample app in the malli codebase)
malli_inst_demo2.mov
Edit:
I realized the same issue will happen with
=>function schemas: once it is registered and instrumented it would always be instrumented. For this use-case I added a dev-time helpermalli.dev.cljs/deregister-function-schemas!which will allow a user to remove all registered schemas to get a clean instrumentation state.