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

ivy runtime error #512

Open
Corfus opened this issue Nov 8, 2019 · 26 comments
Open

ivy runtime error #512

Corfus opened this issue Nov 8, 2019 · 26 comments
Labels

Comments

@Corfus
Copy link

Corfus commented Nov 8, 2019

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

@cormacrelf
Copy link
Owner

angular/angular#25813

@cormacrelf
Copy link
Owner

cormacrelf commented Nov 8, 2019

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.

@ErynManela
Copy link

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 angular/angular#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.

@cormacrelf
Copy link
Owner

This is great, thank you.

@ErynManela
Copy link

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.

@cormacrelf
Copy link
Owner

1.4.0-rc.0

@cormacrelf
Copy link
Owner

#507

@Corfus
Copy link
Author

Corfus commented Dec 5, 2019

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

@cormacrelf
Copy link
Owner

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.

@Corfus
Copy link
Author

Corfus commented Dec 11, 2019

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

I made the PR. #532

@AlbaLopez
Copy link

@cormacrelf when do you think it will be merged?

@cormacrelf
Copy link
Owner

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.

@AlbaLopez
Copy link

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.

@cormacrelf
Copy link
Owner

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.

@initplatform
Copy link

Did I miss the release?

@ErynManela
Copy link

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.

@palpatine1991
Copy link

palpatine1991 commented Mar 27, 2020

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 😞

@ErynManela
Copy link

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.

@palpatine1991
Copy link

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

@trakhimenok
Copy link

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

@acb122
Copy link

acb122 commented Apr 25, 2020

@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.
#571

@palpatine1991
Copy link

palpatine1991 commented Apr 25, 2020

@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

@acb122
Copy link

acb122 commented Apr 25, 2020

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

@MarioC3
Copy link

MarioC3 commented May 13, 2020

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!

@a7md0
Copy link

a7md0 commented Aug 8, 2020

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

@nzbin
Copy link

nzbin commented Aug 8, 2021

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests