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

engine: move static/ contents into public/ and update links, et al. #882

Merged
merged 40 commits into from
Jan 4, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Dec 24, 2019

Per Node 9.0.6+ update
See https://github.com/zeit/next.js/blob/master/errors/static-dir-deprecated.md
And https://nextjs.org/docs#static-file-serving-eg-images

+ Close #804 (adopt restyled.io) using this PR to test it.
+ Fix #854 console errors
+ Close #789

@shcheklein shcheklein temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 24, 2019 02:42 Inactive
just in case? and for consistency with iterative/dvc
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 24, 2019 02:53 Inactive
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Dec 24, 2019

format: add three dashes --- at the beginning of .restyled.yaml a5636b2

That seems to have killed Restyled...

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 24, 2019 03:00 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 24, 2019 03:02 Inactive
Hoping this brings Restyled back to life after it crashes.
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 24, 2019 03:09 Inactive
@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 24, 2019 06:57 Inactive
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Dec 24, 2019

Hey @iAdramelk and @shcheklein I have to ask you for help on this one. And granted, it's not a p1 task or anything, but it's been useful for me to get my hands on the Node/Next code, so your guidance finishing this one would be appreciated.

As you can see here, I moved static/ -> public/ and updated all the imports and links accordingly (see PR description for more context). However, now /doc or other index paths inside aren't working, showing a 404 page (with 200 status) and any valid leaf paths inside also result in Not Found pages (with actual 404 status).

I setup a debugger script in 1fcb0a6 and followed the code in server.js step by step into src/utils/sidebar.js, so I know why we get to these errors, but I'm not sure how exactly. I think it has to do with SSR because server.js is executed 2 (or 3) times, once for pathname "/doc", and again for pathname "/doc/get-started/index.md". The latter value is not found in the sidebar JSON struct by getItemByPath() in sidebar.js, ultimately causing the 404.

My 2 main questions are:

  1. What happens between server.js runs? I don't see a 301 redirection from /doc to /docs/get-started so I guess this is the new SSR "middleware" Alexey implemented. (Where in the src code is that?)
  2. Why is it looking for that pathname on the 2nd run? ("/docs/get-started/index.md") I think it should be looking for just "/docs/get-started" – Does my change from static/ to public/ cause this?

Thanks!

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel marked this pull request as ready for review December 24, 2019 07:47
@iAdramelk
Copy link
Contributor

iAdramelk commented Dec 24, 2019

@jorgeorpinel my wild guess would be that we have problems because of this line:

} else if (/^\/doc.*/i.test(pathname)) {

Basically, I think that at some point in time in the past, the docs URL's were starting with /docs/*, and not /doc/*, it was changed later, and this redirect was added.

And the folder with our docs is called exactly /docs/. In the past, it wasn't a problem because we were using the path /static/docs/, but after moving to /public/ folder it became just /docs/, and now redirects are catching it and forward paths that starting with that to /doc/ folder.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 25, 2019 08:59 Inactive
@jorgeorpinel
Copy link
Contributor Author

our docs is called exactly docs/ ... after moving into public/ its path became just /docs/

🤦‍♂ Yep. I was making complex hypothesis over a routing problem... It's still happening inside the SSR mechanism though, from what I can see in:

dvc.org/pages/doc.js

Lines 144 to 145 in 9ee6eac

try {
const res = await fetch(`${protocol}//${host}${item.source}`)

Anyway, thanks for the educated guess, @iAdramelk! Fixed in 377ab90 🙂

with only default prettier for .md files
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 25, 2019 09:04 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 25, 2019 09:44 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw December 30, 2019 06:01 Inactive
server.js Outdated Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

please, see more comments after the last pass here

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw January 2, 2020 04:09 Inactive
server.js Outdated Show resolved Hide resolved
server.js Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw January 3, 2020 05:22 Inactive
made one of them more lax with i (non-case sensitive)
per #882 (review)
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw January 3, 2020 05:46 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw January 3, 2020 18:09 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorgeorpine-hlkorw January 3, 2020 18:34 Inactive
server.js Show resolved Hide resolved
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Feel free to merge whenever you ready. Good stuff 🎉

@jorgeorpinel
Copy link
Contributor Author

Yay! Merging this will probably create a bunch of merge conflicts in other open PRs though (because of the move from /static/ to /public/static/). Just a heads up guys @ilgooz @pared @iAdramelk @efiop @Suor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: website Area: website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Js console errors/warnings lint: adopt restyled.io app: sort es6 imports
4 participants