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

Navigate using render #374

Merged
merged 8 commits into from
Aug 27, 2019
Merged

Navigate using render #374

merged 8 commits into from
Aug 27, 2019

Conversation

tlgimenes
Copy link
Contributor

@tlgimenes tlgimenes commented Aug 21, 2019

All right, so this is a fun one. This PR entirely rewrites (punch intended) the routing of the web framework, so no need to panic here.

The main ideia behind this PR is to remove all routing related code from runtime and use __pickRuntime from render-server for having a single source of truth for the navigation/rendering.

This is VEEEERY dangerous and should be testes carefully by everyone. Also, releasing the app will increase the traffic both in render and in rewriter, so they need to be scaled up a little bit

@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Aug 21, 2019

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch

  • Minor

  • Major

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

@tlgimenes tlgimenes force-pushed the feature/navigate-render-server branch from 41a0b7b to cef76d5 Compare August 22, 2019 21:41
@tlgimenes tlgimenes changed the title Navigate using render [VERY WIP] Navigate using render Aug 22, 2019
@tlgimenes tlgimenes force-pushed the feature/navigate-render-server branch 3 times, most recently from 571236c to e4f002b Compare August 23, 2019 17:56
@tlgimenes tlgimenes changed the title [VERY WIP] Navigate using render Navigate using render Aug 23, 2019
@tlgimenes tlgimenes force-pushed the feature/navigate-render-server branch from c85f665 to 43718c7 Compare August 24, 2019 18:50
Copy link
Contributor

@brunoabreu brunoabreu left a comment

Choose a reason for hiding this comment

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

60% LGTM
35% TYP
5% Comments to think about

const {
[page]: { allowConditions, declarer },
} = pagesState
const shouldSkipFetchNavigationData =
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a huge improvement when navigating to pages already cached and that don't have conditional content (basically only store routes were declaring allowConditions: true) 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that and added it back

__pickRuntime: runtimeFields,
})
const url = `${path}?${query}`
const page: ServerPageResponse = await fetch(url, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better set credentials to same-origin to ensure we will send cookies.

Also, be careful because fetch will not throw any errors when receiving 4xx or 5xx status code. We should do something like render-session did here.

Another thing to think: fetch will follow redirects by default and we can't do that, we need to handle those redirects by our own to change history and navigate to the redirected page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added render-session code and set credentials to same-origin. However, I could not make the redirects work. I read some of the discussion in this thread and it seems there is no way of handling redirects in JavaScript code when using fetch

@tlgimenes tlgimenes merged commit 02d8ddc into master Aug 27, 2019
@tlgimenes tlgimenes deleted the feature/navigate-render-server branch August 27, 2019 11:27
@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Aug 27, 2019

Your PR has been merged! App is being published. 🚀
Version 8.51.0-beta.2 → 8.51.0

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.

2 participants