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

NavKey navTarget parcelable is not enforced #310

Open
LachlanMcKee opened this issue Jan 2, 2023 · 11 comments · May be fixed by #311
Open

NavKey navTarget parcelable is not enforced #310

LachlanMcKee opened this issue Jan 2, 2023 · 11 comments · May be fixed by #311
Assignees
Labels
api-breaking bug Something isn't working

Comments

@LachlanMcKee
Copy link
Collaborator

At the moment the NavKey navTarget argument is not enforced to be parcelable. It uses @RawValue to avoid the compiler complaining.

Unfortunately this means it is possible to introduce code that will crash at runtime (there is currently an issue in the newly introduced IndexedBackStack causing a crash when the app is backgrounded.

By adding more constraints to the generics we can ensure that we have compile time safety.

@LachlanMcKee LachlanMcKee added the bug Something isn't working label Jan 2, 2023
@LachlanMcKee LachlanMcKee self-assigned this Jan 2, 2023
LachlanMcKee added a commit to LachlanMcKee/appyx that referenced this issue Jan 2, 2023
@LachlanMcKee LachlanMcKee linked a pull request Jan 2, 2023 that will close this issue
2 tasks
LachlanMcKee added a commit to LachlanMcKee/appyx that referenced this issue Jan 2, 2023
LachlanMcKee added a commit to LachlanMcKee/appyx that referenced this issue Jan 2, 2023
@aaudin90
Copy link

aaudin90 commented Jan 9, 2023

Maybe there are options not to use such a restriction? I would like to pass any models to the screen arguments

@LachlanMcKee
Copy link
Collaborator Author

Hi @aaudin90, I'm currently looking into something along these lines.

At the moment if you don't make your model parcelable in some manner your app will crash.

Likely the approach in the short term will be a runtime check earlier to avoid things slipping into production

@aaudin90
Copy link

aaudin90 commented Jan 9, 2023

I understand the solution to the current error. I may be wrong, but having to inherit from parcelable makes appyx less usable.

Is it possible to make a second implementation of BaseNavModel that is not tied to parcelable?

@LachlanMcKee
Copy link
Collaborator Author

Will your Node not be persisting it's routing/state when a configuration change occurs?

@aaudin90
Copy link

aaudin90 commented Jan 9, 2023

You can also get around this by storing the backstack in different stores.

@aaudin90
Copy link

I looked in Decompose, the arguments there must also be Parcelable, but it is possible to disable state saving. And transfer complex models under the annotation @IgnoredOnParcel

@LachlanMcKee
Copy link
Collaborator Author

Thanks @aaudin90. I'll discuss this with the team, and come back to you :)

@aaudin90
Copy link

I will be waiting for you)

@jeddchoi
Copy link

jeddchoi commented Feb 26, 2023

Because of this, :sample:app keeps crashing whenever it goes background with home button or recents button(not back button).

It logs like below.

java.lang.RuntimeException: Parcel: unable to marshal value 0
at android.os.Parcel.writeValue(Parcel.java:1680)
at com.bumble.appyx.core.navigation.NavKey.writeToParcel(Unknown Source:7)
...

@aaudin90
Copy link

aaudin90 commented Mar 9, 2023

@LachlanMcKee, Hi! Have any ideas?)

@aaudin90
Copy link

Perhaps it makes sense to open access to the creation of your own serializers/deserializers, so as not to get tied up in Perselable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants