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

Migrate to more recent SvelteKit #68

Closed
wants to merge 11 commits into from
Closed

Conversation

davej
Copy link

@davej davej commented Dec 6, 2022

This is a follow-on from #67.

  • demo package has to be updated.
  • links resolution is wrong as it's resolving named page routes (i.e., index.svelte instead of +page.svelte). This is mostly in the node/handlers/index.ts file.
  • The glob in sitemap.xml seems to not be picking up an files strangely.

@mihar-22 I'm not quite sure what you mean by "links resolution is wrong"? Could you give more detail and I can try to fix it.

@mihar-22
Copy link
Contributor

mihar-22 commented Dec 6, 2022

Hey @davej thanks heaps for picking this up. I'm sorry I fell behind on this one as there's a lot of work on my plate I need to finish. By links resolution I was referring to this file. It maps file paths in the routes folder (or has that changed?) to a URL and vice versa.

Two functions that need to be checked:

I don't know what the current route file name conventions are but we need to map them. For example, routes/index.md would be /, routes/foo/bar.md would be /foo/bar.

@davej
Copy link
Author

davej commented Dec 6, 2022

@mihar-22 These two functions already seem ok to me but I'm not 100% sure. Can you tell me if the input and output look correct below?

resolveSlug("docs/getting-started/introduction")
// ->
"src/routes/docs/[...1]getting-started/[...1]introduction/+page.md"
slugifyFilePath("/Users/davejeffery/code/forks/kit-docs/packages/kit-docs/src/routes/docs/[...4]production/[...6]deploying/+page.md")
// ->
"/docs/production/deploying"

@davej
Copy link
Author

davej commented Dec 6, 2022

In my latest commit (d5daa84) I managed to get http://localhost:3001/kit-docs/docs.sidebar.json working when there is a +page.md file directly in the root of the docs directory. I did this just by following the execution flow when http://localhost:3001/kit-docs/docs.sidebar.json is requested. But this was a complete guess because I'm missing a lot of context about how this works.

It works for me now @mihar-22 but I'm not sure if it is correct and you are probably best placed to make the last couple of adjustments if they are needed.

@davej
Copy link
Author

davej commented Dec 6, 2022

I am seeing the following error in the console. Any idea what might be going on here?

Error: Module "worker_threads" has been externalized for browser compatibility. Cannot access "worker_threads.resourceLimits" in client code.

@mihar-22
Copy link
Contributor

mihar-22 commented Dec 6, 2022

@mihar-22 These two functions already seem ok to me but I'm not 100% sure. Can you tell me if the input and output look correct below?

Looks good to me.

It works for me now @mihar-22 but I'm not sure if it is correct and you are probably best placed to make the last couple of adjustments if they are needed.

Sure I'll walk through everything when you're done :)

I am seeing the following error in the console. Any idea what might be going on here?

Unfortunately not, probably SvelteKit or Vite related. It depends when that's appearing? What command are you running?

@davej
Copy link
Author

davej commented Dec 6, 2022

@mihar-22 Ah looks like @slowsage might have added a workers_thread import by mistake: https://github.com/svelteness/kit-docs/pull/68/files#diff-2bc97177721c228bd7d17f03d5e7e1690b82a0004f632782adf5c48c890a4d75R2

@davej
Copy link
Author

davej commented Dec 6, 2022

Ok, @mihar-22, I'm finished now. This is ready for review.

@mihar-22
Copy link
Contributor

mihar-22 commented Dec 6, 2022

Awesome thank you so much @davej! I'll have a look either today or tomorrow depending on my work.

@davej
Copy link
Author

davej commented Dec 6, 2022

Thanks. I appreciate it. Looking forward to getting this merged! 😊

@mihar-22
Copy link
Contributor

I think because this was forked from slowsage's fork I can't push to it. I created a new branch pr/67 and with it a new PR.

#69

@mihar-22 mihar-22 closed this Dec 11, 2022
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.

None yet

3 participants