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

fluxible-router: remove route from dehydrated state #670

Conversation

pablopalacios
Copy link
Contributor

Closes #522 (and unsolved #439)

@redonkulus we're facing the issues mentioned in the links above in my company. The summary is:

Given that:

The following problems occur:

  • We are transferring way more data than we need for re-hydration (harming performance)
  • The dehydrated state may even break the application (either by incompatible syntax or by a not found reference)

Previously we were doing some crazy workarounds on our application and I thought it was specific to us. But after checking #522, #439 and fluxible-router codebase, I realized that we could have this fixed in the package itself.

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Besides avoiding unnecessary data to be sent to the client (given that
RouteStore currently doesn't need currentNavigate.route to
re-hydrate), we prevent browsers to break given that
currentNavigate.route.action and currentNavigate.route.handler may
contain server transpiled code that is not compatible with them.
@pablopalacios pablopalacios changed the base branch from master to fluxible-router-1.x November 15, 2020 22:43
Copy link
Member

@lingyan lingyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, especially since RouteStore doesn't need this data to re-hydrate.

@redonkulus redonkulus merged commit 42fcb3c into yahoo:fluxible-router-1.x Nov 17, 2020
@pablopalacios pablopalacios deleted the fix-fluxible-router-dehydration branch November 17, 2020 07:42
@pablopalacios
Copy link
Contributor Author

Thanks for the review :). @lingyan, @redonkulus could we have a 1.x release of fluxible-router?

@redonkulus
Copy link
Contributor

@pablopalacios fluxible-router@1.7.0 published

@redonkulus
Copy link
Contributor

@pablopalacios we probably want to cherry-pick this into the 2.x branch as well

@pablopalacios
Copy link
Contributor Author

@redonkulus yup yup. But I don't know which branch I should base on. fluxible router on master is outdated if compared to the branch we are using for fluxible-addons-react. Since we already released a beta version of both packages, could we merge addons branch on master and keeping moving from there?

@redonkulus
Copy link
Contributor

ya its a little funky right now, I'd say just cherry pick the change into the fluxible-addons-react branch we are using for now. Soon we will be able to merge that into master when we are more comfortable with the changes and migration path.

@pablopalacios pablopalacios restored the fix-fluxible-router-dehydration branch December 9, 2020 12:29
@pablopalacios pablopalacios deleted the fix-fluxible-router-dehydration branch May 13, 2021 09:17
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 this pull request may close these issues.

dehydrated value of RouteStore contains action value [fluxible-router v1]
3 participants