angular-skyhook icon indicating copy to clipboard operation
angular-skyhook copied to clipboard

ivy runtime error

Open Corfus opened this issue 4 years ago • 26 comments

Skyhook works perfect with the actual default angular compiler, but when you change the compiler to ivy, you get this runtime error.

It seems to be a problem with the directives and ivy.

Capture

Corfus avatar Nov 08 '19 11:11 Corfus

https://github.com/angular/angular/issues/25813

cormacrelf avatar Nov 08 '19 16:11 cormacrelf

Ivy is meant to be backwards compatible, although I am sure there must be caveats, as nothing in JavaScript is actually that easy. In any case, I'm going to assume Ivy-related bugs are on Ivy's side until convinced otherwise, and do close to zero investigation. You can leave it open in case anyone else comes across it.

cormacrelf avatar Nov 08 '19 17:11 cormacrelf

the injection context is a breaking change. however if someone wants to use your existing build they run the basic postinstall script described here on a project that depends on angular-skyhook, it should work: https://next.angular.io/guide/migration-ngcc

"postinstall": "ngcc --properties es2015 browser module main --first-only --create-ivy-entry-points" this should do all the conversion for anyone using the library.

That said, when you do that the other postinstall script they mention, the for use with nguniversal, "postinstall": "ngcc --properties main --create-ivy-entry-points" fails if using @angular-skyhook/dnd-multi-backend due to https://github.com/angular/angular/issues/33697

This and a number of other number of breaking changes ng update will fix for you and leave the library in a state that is still backwards compatible.

ErynManela avatar Nov 08 '19 22:11 ErynManela

This is great, thank you.

cormacrelf avatar Nov 08 '19 23:11 cormacrelf

Once you pull in the dnd-multi-backend v5 update the ngcc error should go away because the rollups are better formatted. That doesn't make it natively ivy compatible, but it will make it useable. When is that branch going to be ready? Could you make a beta release off that branch for testing purposes? It would be very helpful.

ErynManela avatar Nov 11 '19 13:11 ErynManela

1.4.0-rc.0

cormacrelf avatar Nov 13 '19 09:11 cormacrelf

#507

cormacrelf avatar Nov 13 '19 10:11 cormacrelf

I found the problem with ivy. In Ivy you have to inherit from another directive. So your abstract class DndDirective doesn't work with ivy, if you adapt it to a real Directive it should be ivy compatible, see here:

https://next.angular.io/guide/ivy-compatibility-examples#undecorated-classes

In my fork it is fixed: https://github.com/Corfus/-angular-skyhook-ivy/tree/cdb/ivy-compatibility-fix

Corfus avatar Dec 05 '19 16:12 Corfus

Is that all that needed to be done?

In any case, can you make a PR? A fork is cool, but a PR actually helps land the fix.

cormacrelf avatar Dec 11 '19 07:12 cormacrelf

Yes with this adaptions it works without distinctions with ivy, too.

I made the PR. https://github.com/cormacrelf/angular-skyhook/pull/532

Corfus avatar Dec 11 '19 08:12 Corfus

@cormacrelf when do you think it will be merged?

AlbaLopez avatar Dec 11 '19 11:12 AlbaLopez

I think I’ll wait about 2 years, during which time I will get to some of the little things I’ve always wanted to do, like cultivate a tasteful indoor-plant aesthetic, whittle a log into the shape of a realistic Christmas ham, and train as a spider milker at the local zoo.

cormacrelf avatar Dec 11 '19 14:12 cormacrelf

I think I’ll wait about 2 years, during which time I will get to some of the little things I’ve always wanted to do, like cultivate a tasteful indoor-plant aesthetic, whittle a log into the shape of a realistic Christmas ham, and train as a spider milker at the local zoo.

A somewhat exaggerated answer to a question without bad intention.

Good luck and follow your dreams.

AlbaLopez avatar Dec 11 '19 14:12 AlbaLopez

I had hoped that merging the PR immediately would let me joke about it, but apparently no. Back to being a maintenance robot it is.

cormacrelf avatar Dec 11 '19 14:12 cormacrelf

Did I miss the release?

initplatform avatar Jan 20 '20 03:01 initplatform

Yay angular 9 is out! finally! I'm sticking with ViewEngine for a few months before switching to Ivy, but following this thread to see how much spider milk @cormacrelf has made. I'd like to see a picture of the ham log, too.

ErynManela avatar Feb 07 '20 15:02 ErynManela

Any update to this? ngcc advice from @aaronmanela did not help to me and I found it impossible to use the skyhook with Angular 9 (with IVY enabled) so far 😞

palpatine1991 avatar Mar 27 '20 07:03 palpatine1991

Did you try using 1.4.0-rc0 branch @palpatine1991 ?

Also the ngcc postinstall fixes will no longer be recommended in Angular 9.1 as they have made it better.

ErynManela avatar Mar 28 '20 16:03 ErynManela

Yes, I am using skyhook 1.4.0-rc0 and I have tried it even with Angular 9.1.0. Still have the same error

palpatine1991 avatar Mar 30 '20 10:03 palpatine1991

@cormacrelf do you plan to update examples to Angular 9.* with Ivy enabled? The "@angular-skyhook/core": "^1.4.0-rc.0" does not work with Ivy enabled for me.

trakhimenok avatar Apr 09 '20 11:04 trakhimenok

@cormacrelf I have made a PR which fixes the library for IVY. Could you please review it, happy to take any feedback on it, you will see it is very basic fix to make the ngcc compiler happy. https://github.com/cormacrelf/angular-skyhook/pull/571

acb122 avatar Apr 25 '20 12:04 acb122

@acb122 is the core of your fix adding the missing exports, upgrading dependencies or disabling the IVY?

I mean if IVY disabling is not a core of the fix, I would consider to keep it on because of future maintainability

palpatine1991 avatar Apr 25 '20 13:04 palpatine1991

@palpatine1991 the core fix is the missing exports.

IVY disabled is the current recommended setting in angular libraries in angular v10 that will change.

I have removed my upgrading angular change as that broke tests.

acb122 avatar Apr 25 '20 15:04 acb122

Hey @cormacrelf, first of all, thanks for maintaining this library! I was wondering when will you be able to merge PR #571 made by @acb122 ? We're having problems with Angular 9 and this library!

Thanks again!

MarioC3 avatar May 13 '20 02:05 MarioC3

Is there any update on this? Have anyone published #571 to new NPM package

a7md0 avatar Aug 08 '20 21:08 a7md0

@palpatine1991 @acb122 @aaronmanela @MarioC3 @a7md0 I have published the fixed version to npm, everything is fine with angular ivy. @ng-dnd/core @ng-dnd/multi-backend @ng-dnd/sortable

nzbin avatar Aug 08 '21 03:08 nzbin