Skip to content

feat: use history mode and deprecate hash routing by pre-rendering pages #494

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

Closed
wants to merge 10 commits into from

Conversation

zebateira
Copy link
Contributor

@zebateira zebateira commented Jul 22, 2020

Closes #105 🎉
Unblocks #258 🎉

This PR removes hash routing by using history: 'mode' and pre-renders the pages as suggested by @mikeal

This should be ready to go, but we need to discuss the bellow caveats and understand if they are blockers to release this.

Changes

  • Update all links to the new routes (removing #/)
  • Add prerender-spa-plugin (only turned on in production to not slow down dev builds and hot reload)
  • Add redirect from hash routes to normal routes
  • Replace vue-monaco-editor (deprecated) with monaco-editor-vue
  • Change api strategy to not use promises so we can use it in the vue.config.js file (since await is not supported in node in the current version). This affects the protowizard, so I updated it and the tests.

Caveats

  • The monaco editor does not support using multiple editors with different themes at the same time, so the main code editor and the solution code need to be in the same theme. Before it was working most likely because the strategy of the previous package was not very efficient and it was loading separate instances or maybe simply upgrading from 0.6 to 0.20 of the monaco-editor package. Issue: monaco-editor/#338
  • Autocomplete on the code editor stopped working. I don't think this is a deal breaker considering it was not as useful as it could be before, but I'll try to fix it and actually have autocomplete for ipfs and such in another PR.
  • When the user changes the filter in the tutorials page, the query parameters will trigger a window.location change, so the window will scroll to the top. Before we were able to bypass that issue because we were using hash routing. But now, the change in window.location.search instead of window.location.hash triggers a scroll to top. To try and minimize this I manually restored the scroll when we change the quey parameters, but sometimes it still flickers.

@zebateira zebateira requested a review from terichadbourne July 22, 2020 20:29
@terichadbourne
Copy link
Member

Thanks, @zebateira! I'll take this for a spin tomorrow and would love a tour at some point this week, but a few quick questions based on your description of the changes and caveats:

Change api strategy to not use promises so we can use it in the vue.config.js file (since await is not supported in node in the current version)

  • Does this mean await is not supported in the current version of the pre-rendering package?
  • Does this only affect us as authors of the platform code, and the learners can still use promises in the code that they write to solve challenges?
  • Does this affect the way we communicate with IPFS behind the scenes or just the way we use the internal APIs we built to help our Vue files talk to our JSON data?

Add redirect from hash routes to normal routes

  • Is this just a one-time process for any routes we currently use?
  • Do the routing changes affect the feature that lets us change the name and URL of a tutorial and have visitors the old URL be redirected?

The monaco editor does not support using multiple editors with different themes at the same time

  • Is the main impact of this that we can't have the challenge box be on a white background and the solution on black? Or can we not have one editable and one non-editable, for example?
  • What are your biggest concerns here from the learner's perspective?

Now the change in window.location.search instead of window.location.hash triggers a scroll to top. To try and minimize this I manually restored the scroll when we change the query parameters, but sometimes it still flickers.

  • Are users likely to see this flicker only on our tutorials page when filtering our content, or am I forgetting other spots where we're using query parameters and will be affected?
  • This change doesn't affect our ability to cache filtering choices and pass tracking events to Countly based on query parameters, right?

@zebateira
Copy link
Contributor Author

zebateira commented Jul 23, 2020

Change api strategy to not use promises so we can use it in the vue.config.emphasized file (since await is not supported in node in the current version)

Sorry, my bad, I was not descriptive enough here: I mean to say that await is not supported in node in the global scope/top-level in the current version, so the correct answer to your question is: it only affects the way we use the internal APIs we built to help our Vue files talk to our JSON data.
Basically, I had to get the routes for the tutorials and the lessons (/basics, /basics/01, /blog/03, etc) and pass them to the prerender plugin in the vue.config.js file. But because this is a node file in the global scope/top-level, async/await is not supported in this version (only node v14), so I had to convert everything to sync to make it work. Ref: https://v8.dev/features/top-level-await

Add redirect from hash routes to normal routes
Is this just a one-time process for any routes we currently use?

What do you mean by one-time process?
The redirect will be performed every time someone wants to access some path that contains the hash routes, e.g. proto.school/#/tutorials redirects to proto.school/tutorials

Do the routing changes affect the feature that lets us change the name and URL of a tutorial and have visitors the old URL be redirected?

No, it will work as expected. The redirect still happens because it was done under the hood by vue-router. Even if you access an old URL via hash route, you will be correctly redirected.

The monaco editor does not support using multiple editors with different themes at the same time
Is the main impact of this that we can't have the challenge box be on a white background and the solution on black? Or can we not have one editable and one non-editable, for example?

The former. Because if you change the theme in the first editor, the second will also change. So we have to choose one theme that will be applied to both.

What are your biggest concerns here from the learner's perspective?
My concern here is the fact the color differences between the two code blocks, code editor and read-only code solution, are emphasized by the color theme.

Now the change in window.location.search instead of window.location.hash triggers a scroll to top. To try and minimize this I manually restored the scroll when we change the query parameters, but sometimes it still flickers.
Are users likely to see this flicker only on our tutorials page when filtering our content, or am I forgetting other spots where we're using query parameters and will be affected?
Only the tutorials page as far as I can remember as well.

This change doesn't affect our ability to cache filtering choices and pass tracking events to Countly based on query parameters, right?
No, everything is still working as expected.

@zebateira
Copy link
Contributor Author

zebateira commented Jul 23, 2020

The monaco editor does not support using multiple editors with different themes at the same time

I just had a crazy idea tho: using filter: invert(1) to change the theme 🤣
By applying this: filter: invert(1) hue-rotate(170deg)
We get this:

Screenshot 2020-07-23 at 10 07 37 AM

For comparison, this is the current live version:
image

Support for the filter function is pretty good.

I pushed this change. @terichadbourne let me know if you think it looks ok.

@terichadbourne
Copy link
Member

Thanks for the clarifications!

The filter trick looks great based on your screenshots. Very sneaky. 🤓

What do you mean by one-time process?
The redirect will be performed every time someone wants to access some path that contains the hash routes, e.g. proto.school/#/tutorials redirects to proto.school/tutorials

Sorry, I wasn't clear. I was asking whether you were making a list of all the URLs that currently exist (hash ones) and their corresponding hashless ones (in which case the list doesn't get updated later, we just create it once), or doing something programmatically forever to remove hashes from URLs for everything, even if those pages don't exist yet. Sounds like the latter.

So related question... will pulling the hashes out like this prevent us from using anchor tags moving forward? (Haven't been able to yet, but I've been excited to do it.) I assume anchor tags won't have a slash immediately after them, so if we're only removing #/ and not # maybe this isn't a problem?

@terichadbourne
Copy link
Member

Oh, one other related question that would need to be addressed outside of the PR. Is there any way we can retroactively fix existing data in Countly so that for things segmented by paths we can find one entry for a page instead of looking for a stat from when we had hash mode and a stat from when we had history mode and adding them together?

@zebateira
Copy link
Contributor Author

Sorry, I wasn't clear. I was asking whether you were making a list of all the URLs that currently exist (hash ones) and their corresponding hashless ones (in which case the list doesn't get updated later, we just create it once), or doing something programmatically forever to remove hashes from URLs for everything, even if those pages don't exist yet. Sounds like the latter.

Yes, the latter. Here is the change that performs the redirect: https://github.com/ProtoSchool/protoschool.github.io/pull/494/files#diff-1c90ff38a08209f9ebd4d05d1e43358eR34

So related question... will pulling the hashes out like this prevent us from using anchor tags moving forward? (Haven't been able to yet, but I've been excited to do it.) I assume anchor tags won't have a slash immediately after them, so if we're only removing #/ and not # maybe this isn't a problem?

If you check the above redirect code change we only remove and redirect paths that contain hashes that start with #/.
So now we are able to use anchor tags to our hearts desires 🎉

@zebateira
Copy link
Contributor Author

Oh, one other related question that would need to be addressed outside of the PR. Is there any way we can retroactively fix existing data in Countly so that for things segmented by paths we can find one entry for a page instead of looking for a stat from when we had hash mode and a stat from when we had history mode and adding them together?

I'll look into this 👍

@zebateira
Copy link
Contributor Author

zebateira commented Jul 24, 2020

Note: add tests to check the redirects

Update: done! 9eeb443

@zebateira zebateira force-pushed the fix/pre-render branch 2 times, most recently from 412b96f to 254147e Compare July 29, 2020 17:17
@zebateira
Copy link
Contributor Author

Closed since main is not deployed and contains these changes.

@zebateira zebateira closed this Aug 3, 2020
@zebateira zebateira deleted the fix/pre-render branch August 3, 2020 13:51
@zebateira
Copy link
Contributor Author

Oh, one other related question that would need to be addressed outside of the PR. Is there any way we can retroactively fix existing data in Countly so that for things segmented by paths we can find one entry for a page instead of looking for a stat from when we had hash mode and a stat from when we had history mode and adding them together?

Update on this: the countly team has migrated our data, and so previous /#/* routes now match /* without the hash.

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.

Nicer URLs :)
2 participants