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

Advanced optimization can break due to compiler hack being overwritten #880

Open
awkay opened this issue Jul 15, 2017 · 7 comments
Open

Comments

@awkay
Copy link
Contributor

awkay commented Jul 15, 2017

Using cljs 1.9.671 and om-beta1. Have verified in the compiled js (optimization none) that static methods are not getting @nocollapse, and are be elided by Closure.

Using cljsbuild 1.1.6 to build.

@awkay
Copy link
Contributor Author

awkay commented Jul 15, 2017

Initial investigation shows that the intern hack in next.cljc is not taking effect. I've checked my classpath to ensure there isn't an AOT compiler being used, and everything looks right.

@swannodette
Copy link
Member

@awkay this sounds like a regression in ClojureScript?

@awkay
Copy link
Contributor Author

awkay commented Jul 16, 2017

No, Antonio had put a hack into next.cljc that hacks the compiler to add nocollapse. The hack seems to not be working anymore, but I can't see where it is going wrong. It isn't getting installed anymore, and I'm not getting an AOT compiler on my classpath.

It seems to me that this is certainly something that could be fixed VIA the compiler proper, but I'm not sure if it would be purely for the Om Next users, since "static" additions to protocols is at the heart of it.

@awkay
Copy link
Contributor Author

awkay commented Jul 16, 2017

Location of hack: https://github.com/omcljs/om/blob/master/src/main/om/next.cljc#L313

@awkay
Copy link
Contributor Author

awkay commented Jul 20, 2017

OK, I tinkered some more with this. when I pare down the project to nothing but Om Next, then the correct @nocollapse annotations appear. So, my suspicion that the compiler hack is getting short-circuited are correct. Is there some way this can be moved into the compiler proper?

@awkay
Copy link
Contributor Author

awkay commented Jul 25, 2017

I tracked down the specific issue, and I was correct in my assessment: something was stomping on the hack. Our production compile had namespace refresh tools on the classpath that were triggered once on initial load which was causing the compiler, but for some reason not Om, to get refreshed in the runtime (possibly just out of order).

So, I still consider this a problem in Om, since a library should not hack the compiler...but I've at least found a solution to my specific instance of it going wrong.

@awkay awkay changed the title Advanced optimization seems to have broken Advanced optimization can break due to compiler hack being overwritten Dec 21, 2017
@awkay
Copy link
Contributor Author

awkay commented Dec 21, 2017

I fixed this in Fulcro by changing defui to emit a try/catch of calls to the protocol methods via the class. This let's Closure understand that they are needed, and prevents them from being removed without having to hack into the cljs compiler.

The fix is this, which may apply cleanly as a patch to Om Next:

fulcrologic/fulcro@93754c8

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

2 participants