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

Navigation types #6517

Closed
Rich-Harris opened this issue Sep 1, 2022 · 10 comments
Closed

Navigation types #6517

Rich-Harris opened this issue Sep 1, 2022 · 10 comments
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

Relevant: #4447, #4383, #6498.

The navigating store value is a { from: URL, to: URL } object, which is also passed to beforeNavigate (along with a cancel method) and afterNavigate callbacks.

Often, it's useful to know what type of navigation is happening — whether we're coming from or going to a different document, or we navigated within the site to a different route by clicking on a link, or via goto, or by a popstate event (i.e. traversing the history). If it's a history traversal, it's also useful to know the delta (or at least whether we're going forward or back).

Describe the proposed solution

Since the types are mutually exclusive, it probably makes sense to be an enum, rather than

{ load: true, goto: false, ... }

or something of that nature. Delta can be reported in a separate property that would only apply in the popstate case:

beforeNavigate(({ from, to, type, delta }) => {
  if (delta) {
    console.log(`travelling ${delta < 0 ? 'backwards' : 'forwards'} through time`);
  } else switch (type) {
    case 'unload': console.log('bye for now'); break;
    case 'click': console.log('clickety clack'); break;
    case 'goto': console.log('as you wish'); break;
  }
});

(load isn't pictured because it would never be fired in the beforeNavigate case, just like unload would never appear in afterNavigate.)

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

@Rich-Harris Rich-Harris added this to the 1.0 milestone Sep 1, 2022
@Conduitry
Copy link
Member

Something I'd floated at the last maintainers meeting was the possibility of invalidate() being a type of navigation. It's not really, but the load function is running, and it might be nice to be able to decide to display a loading animation during this, which I don't think is currently possible in a nice way. Is this more palatable if we have different flags for different types of navigation? Do we want to call the store something else that encompasses more than just "navigating"?

And, relatedly, is this proposal, as currently written, something that would apply just to the lifecycle hooks, or would it be for the $navigating store values as well?

@Rich-Harris
Copy link
Member Author

Rich-Harris commented Sep 1, 2022

It would apply to $navigating as well.

I'm slightly hesitant to lump invalidation in here as well — it feels like you might want more of a separation between the concepts (and should it be possible to cancel() an invalidation in beforeNavigate?). Then again, type === 'invalidation' is much less new surface area than beforeInvalidate, afterInvalidate and $invalidating.

Someone tell me the correct answer please

@madeleineostoja
Copy link

madeleineostoja commented Sep 1, 2022

I haven't used invalidate extensively, just in a crude manner to nuke my whole data stack when something like a logout happens, but I would find it extremely odd if you could cancel an invalidation in beforeNavigate. That said, it kind of also makes sense in my brain for it to be a type so you could do any other cleanup/reactions you need there?

Would it be an unacceptable caveat to make cancel just not apply to type === 'invalidate'? Or (and I feel this prob goes against the patterns established by the rest of kit), but could invalidate just take a callback to handle before/after/etc?

@f-elix
Copy link
Contributor

f-elix commented Sep 2, 2022

Would it also be a good idea to add a 'submitting' type when the upcoming Form component is used? I know that Remix has something like that.

As for invalidation, I think its a good idea to include it, and being able to cancel it makes sense to me (cancelling the fetch requests and the data update), albeit maybe a rare use case.

@Rich-Harris
Copy link
Member Author

Yeah, I think it would be weird to include invalidation but disallow cancel, even if it would only be used rarely.

Am more getting hung up on whether it makes sense to combine the 'navigation' and 'invalidation' concepts. They definitely have overlap (and implementation-wise are very similar), am just trying to figure out if the differences are likely to cause issues down the line.

For example in #5478 (comment) I suggest including a snapshot in afterNavigate, so that you can do things like reset scroll positions, repopulate form controls etc. Should snapshot be present if type === 'invalidate'? Or does it not really matter, since you can always know if afterNavigate was triggered by an invalidation?

I'm leaning towards type === 'invalidation', but want to make a decision we don't later regret

could invalidate just take a callback to handle before/after/etc?

How would you envisage using it? Presumably anywhere you're in a position to pass in a callback you could already do this:

before();
await invalidate(...);
after();

@Rich-Harris
Copy link
Member Author

Would it also be a good idea to add a 'submitting' type when the upcoming Form component is used?

I think that's better off living in the component than in the framework — you could have multiple forms submitting simultaneously, for example

@dummdidumm
Copy link
Member

This is great! Only thing I'm not sure about yet is invalidation. There are good reasons to treat it as a navigation:

  • If there was no JavaScript, you'd have to do a full page reload which is kind of a navigation
  • Under the hood it calls update which is part of navigation
    Still feels kinda off, and I'm not sure if it could get annoying if you have to guard all your navigations with if (type !== 'invalidation')

@lukaszpolowczyk
Copy link
Contributor

@dummdidumm Maybe instead of if (type !== 'invalidation'), give a

let includeInvalidate = true; 
beforeNavigate(({ from, to, type }) => {}, includeInvalidate);

?

Or a separate function that includes invalidate....

@aradalvand
Copy link
Contributor

Also related: #6375

@madeleineostoja
Copy link

I'm not sure if it could get annoying if you have to guard all your navigations with if (type !== 'invalidation')

I think this would unquestionably get annoying, and moreover be an easy footgun

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

Successfully merging a pull request may close this issue.

7 participants