Skip to content

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.

Details

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:

https://github.com/iterative/dvc.org/blob/5b3bf770c6261e88d3832bf3315922b7e289b42a/server.js#L98

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:

https://github.com/iterative/dvc.org/blob/9ee6eac5baa0bf952ff4a6a9d216041c56e63299/pages/doc.js#L144-L145

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
Copy link
Contributor

@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
@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
Copy link
Contributor

@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