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 SvelteKit #311

Open
benmccann opened this issue Oct 11, 2021 · 40 comments · May be fixed by #349
Open

Migrate to SvelteKit #311

benmccann opened this issue Oct 11, 2021 · 40 comments · May be fixed by #349

Comments

@benmccann
Copy link

Sapper is no longer actively developed and SvelteKit is a much better experience

Migration guide: https://kit.svelte.dev/migrating

@illright
Copy link
Owner

Hi! We have considered it. In regards to moving the repository to the SvelteKit-proposed "lib for source, routes for docs", we have faced difficulties with migration. Something was off with the packaging. Or something, honestly, don't remember now (should have taken notes!). It didn't seem like a good experience back then, that was around a few months ago.

Alternatively, we tried replacing Sapper with SvelteKit just for the docs and leaving the rest of the library as it is. We've managed to fix the issues that came up, but never got around to finishing it. Still, the experience of migrating to SvelteKit leaves things to be desired. I wouldn't call it a smooth migration just yet.

That said, we are watching SvelteKit closely and will hop over as soon it becomes sufficiently stable.

@benmccann
Copy link
Author

I guess by "lib for source", you mean svelte-kit package? I was only thinking of the docs site, since that was the one place I'd seen Sapper being used

Do you happen to have a branch where you're working on the docs migration?

I'm one of the primary maintainers of SvelteKit and looking for any issues that need to be fixed before 1.0 since we're getting closer to that. I went through all the open source repos using Sapper and left this message with any large, active repos to help discover what blockers there might be. I'd want to make sure we have any issues fixed before declaring 1.0. We just migrated sveltesociety.dev and have a PR open for svelte.dev and were able to migrate those two, but I'm curious to see if there are issues that others might be facing out in the wild, so I'm happy to help if there are things that might be causing any difficulty.

@illright
Copy link
Owner

Yeah, ideally we'd like to make use of svelte-kit package and have the library and the docs be a single Svelte app. But that's not happening for now.

As for the branch – yes, I just pushed the existing migration work, you can see the pull request linked below. I notice a bit of jank when navigating the pages – whatever might be causing that. Also I feel like configuring the app to export statically might not be as straightforward as it was with Sapper.

As for other issues that I've encountered – a known issue where Vite doesn't do define-substitution in Svelte files. Worked around that by defining the substituted constants in a separate file.

@benmccann
Copy link
Author

I notice a bit of jank when navigating the pages – whatever might be causing that.

There's some issues in dev mode that might cause a flash of unstyled content or other jankiness, but you won't see this in production. You do have an issue with loading your style sheets though - I left a comment on the PR about that

Also I feel like configuring the app to export statically might not be as straightforward as it was with Sapper.

I'm not sure if there's some reason you expect it might be difficult, but in general all you need to do is install the static adapter, which is pretty straightforward: https://github.com/sveltejs/kit/tree/master/packages/adapter-static#usage

I can give you more advice if you run into trouble or have a certain web host you're targeting

As for other issues that I've encountered – a known issue where Vite doesn't do define-substitution in Svelte files. Worked around that by defining the substituted constants in a separate file.

That should work by default soon. We have a PR out to start to address it: sveltejs/svelte#6835

Let me know if there's anything else I can do to help!

@illright
Copy link
Owner

When I mentioned jank a few days ago, I was recalling from memory. Today I went ahead and tried it again.

The Vite server starts almost instantly, which is great, but then the first request to localhost takes around 10-15 seconds to complete. If I then hit Reload, the reloads will also take a long time, sometimes even longer than the initial load. Sometimes during reloads, I get random 500 errors, sometimes only in the tab title, sometimes they break through to the screen. The error message is TypeError: Failed to fetch dynamically imported module with no helpful logs (not even a 500 report) in the Node console. Sometimes Chrome reports the page to be unresponsive. The "loading page" spinner in the tab gets visibly janky, my laptop fans engage as if the browser is struggling to load the page. Hitting the "stop loading" cross icon doesn't stop the spinner from spinning, it's as if the tab is now so broken that it doesn't respond to Chrome anymore.

This amounts for a rather unpleasant development experience. I think we should stay with Sapper until this performance issue gets resolved. I'm happy to aid in investigating that issue.

@illright
Copy link
Owner

illright commented Oct 14, 2021

Also, minor comment. The SvelteKit's text in the console is black, which is rather hard to read on dark terminals:
image
Unlike Vite, which produces more friendly looking output. Also they duplicate each other, I think only one should stay (and right now I'd rather it be Vite's)

@benmccann
Copy link
Author

they duplicate each other, I think only one should stay (and right now I'd rather it be Vite's)

That's been fixed. Please upgrade to the latest version of SvelteKit

The colors thing is weird. The text is not black for me, but is grey. Maybe it's something to do with us having different terminal themes

Screenshot from 2021-10-14 11-16-16

@illright
Copy link
Owner

Uh-huh, I updated and now the duplication is gone. The performance issue still persists though.

What was black for me then is still black now. Moreover, I've noticed that tracebacks are also printed in this color:
traceback

Maybe we could use a theme-independent color here?

@benmccann
Copy link
Author

Sometimes during reloads, I get random 500 errors, sometimes only in the tab title, sometimes they break through to the screen.

This seems to be due to broken links. It appears we don't give very good error messages when trying to prefetch a page that doesn't exist. If you're able to fix all the links first then I can take another look at any remaining jank or performance issues

E.g. if you're on http://localhost:3000/docs/components/button and click "migration guide" you will be taken to http://localhost:3000/docs/components/docs/migration-guide instead of http://localhost:3000/docs/migration-guide.

Uh-huh, I updated and now the duplication is gone

Did you just do that locally? I don't see the changes appearing

What was black for me then is still black now.

We print it in gray: https://github.com/sveltejs/kit/blob/7fab7f67a4643715586693cf01fd4d43f54671d0/packages/kit/src/cli.js#L238

If your terminal theme is showing gray as black, it seems more to me like the bug would be in your terminal theme. I'm hesitant to try to work around that in SvelteKit because there's an infinite number of terminal themes out there and if we fix it for one then we'll probably break it for another. I haven't seen anyone else hit this issue before

@illright
Copy link
Owner

Fixed the links, supposedly. The jank is still there, on any page, even the /, where the links would work anyway. I don't think the links were the issue.

Did you just do that locally?

Yeah, but now I pushed the changes.

@benmccann
Copy link
Author

I cannot reproduce any error when reloading the page. I can't find any open issues about it either. Enough people are using SvelteKit at this point that it has to be something specific to your project or more likely your machine since I can't reproduce it.

Can you run npx envinfo --system --binaries --browsers --npmPackages "{svelte,@sveltejs/*,vite}" and share the results? I wonder what is different between our two machines.

I did encounter two errors on the console while clicking around. It looks like you have two file paths that didn't get updated:

[svelte-preprocess] The file  "/home/bmccann/src/tmp/attractions/docs/static/css/routes/docs/components/autocomplete.scss" was not found.
[svelte-preprocess] The file  "/home/bmccann/src/tmp/attractions/docs/static/css/routes/docs/components/file-tile.scss" was not found.

@aabounegm
Copy link
Collaborator

I also can't reproduce any errors. I thought I'd give it a shot, since my OS is quite different than @illright's (I use Windows 10, he uses Linux and I forgot which distro 😁). To make it work, I first had to replace any usages of the custom resolve function from resolve.js with the one built-in in Node, while also removing the import.meta.url part. Then it worked without further modifications.

About the 2 errors, it is not that the file path has not been updated, but rather they were missed in the commit where all the styles where inlined. In fact, there are 5 of them and I believe they are a problem of copy-pasting files, forgetting to remove the style import, the styles not applying due to different selectors, and not getting any errors earlier due to the files existing. None of that is SvelteKit's problem, but I'm just writing my diagnosis of the issue so that we (@illright) can fix them.

As for the issues described above, everything is working very smoothly. Yes, the initial page load is relatively slow, but I understand that's due to how Vite works, with all the compiling on demand. After the initial load, I get no more jittering. Also, all the colors look good on my terminal with a white background.

System info
System:
    OS: Windows 10 10.0.19043
    CPU: (8) x64 Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
    Memory: 6.24 GB / 15.84 GB
Binaries:
    Node: 16.9.1 - C:\Program Files\nodejs\node.EXE
    npm: 7.21.1 - C:\Program Files\nodejs\npm.CMD
Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (94.0.992.47)
    Internet Explorer: 11.0.19041.1202
npmPackages:
    svelte: ^3.43.2 => 3.43.2

@benmccann
Copy link
Author

Thank you for testing! I'm also on Linux, for what it's worth. Maybe @illright should run git clean -xdf to blow away all the built files and see if that helps?

Regarding import.meta.url, please see sveltejs/kit#2604 (comment). Possibly it would be fixed in 1.0.0-next.184 but I have not tested. It looks like the PR branch is on 183, so it could be worth updating

@aabounegm
Copy link
Collaborator

import.meta.url seems to work correctly. The problem is in how it is used, and that is with the new URL constructor. The returned url is invalid on Windows since it starts with a forward slash. For example, this is what I get when I call resolve(import.meta.url, './src/lib/mdsvex/layout.svelte') (as was in mdsvex.config.js): /D:/Projects/attractions/docs/src/lib/mdsvex/layout.svelte. Obviously, this path is invalid on Windows because of the initial slash (Windows can handle forward slashes in place of backslashes as the separator, but not as the first character in the path).
Again, this is a problem with the custom resolve function, and was fixed by replacing import resolve from './resolve.js'; with import { resolve } from 'path';

@illright
Copy link
Owner

illright commented Oct 15, 2021

My system info, where I get the lags
System:
  OS: Linux 5.14 Manjaro Linux
  CPU: (8) x64 Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
  Memory: 1.35 GB / 7.62 GB
  Container: Yes
  Shell: 3.3.1 - /usr/bin/fish
Binaries:
  Node: 16.10.0 - /usr/bin/node
  npm: 7.24.1 - /usr/bin/npm
Browsers:
  Firefox: 93.0
npmPackages:
  @sveltejs/kit: next => 1.0.0-next.184 
  svelte: ^3.43.2 => 3.43.2

I've also brought up an Amazon EC2 instance with Attractions docs in dev mode, upd: no longer available. Let me know if it's needed and I'll bring it back up again. It seems a bit better (not seeing 500's anymore), but still janky, as in the reloads take a long time, not only the inital page load.

AWS EC2's system info
System:
  OS: Linux 4.19 Debian GNU/Linux 10 (buster) 10 (buster)
  CPU: (1) x64 Intel(R) Xeon(R) CPU E5-2676 v3 @ 2.40GHz
  Memory: 86.10 MB / 987.52 MB
  Container: Yes
  Shell: 5.0.3 - /bin/bash
Binaries:
  Node: 16.11.1 - /usr/bin/node
  npm: 8.0.0 - /usr/bin/npm
npmPackages:
  @sveltejs/kit: next => 1.0.0-next.183 
  svelte: ^3.43.2 => 3.43.2

@benmccann
Copy link
Author

benmccann commented Oct 15, 2021

The slow reloads seems to be because it's loading 485 files in dev mode. It appears to be loading all of svelte-feather-icons when you use just a few icons. Vite is supposed to do dependency pre-bundling to avoid this: https://vitejs.dev/guide/dep-pre-bundling.html#the-why. However, Vite's pre-bundling uses esbuild, so can't pre-bundle packages holding Svelte components because it doesn't know what to do with .svelte files

In production mode, it's not bad. It only loads 33 files and is pretty snappy. Production mode build fails by default though. I sent a fix for that here: lgarron/clipboard-polyfill#144

@illright
Copy link
Owner

Thank you for a thorough investigation :)

I believe it was once a best practice for Sapper to install Svelte packages to development dependencies for SSR. I have a lot of questions:

  1. Is it still the case?
  2. Is that why svelte-feather-icons are causing trouble?
  3. Do we as library maintainers have to do anything about the way we ship Attractions to make sure it's snappy for our users?
  4. If bundled versions of libraries are used, does that mean that our customisation scheme of Attractions will no longer work in SvelteKit?

@benmccann
Copy link
Author

Putting it dev dependencies is still a good place to put it. It won't affect this at all

I actually have no idea how to fix this in Vite. It will probably be extremely hard to fix and not happen anytime soon. In the meantime, what I would recommend is doing a deep import as a workaround. It looks like that was recommended even in Sapper actually: dylanblokhuis/svelte-feather-icons#10 (comment)

You should not bundle the library ahead of time (ignore that I said that. I edited my comment above)

I think your users may have to do deep imports of attractions for the meantime as well if performance is an issue for them. I hate having to recommend that, but I'm afraid it's the best I have at the moment.

@illright
Copy link
Owner

I've implemented a preprocessor that does rudimentary import re-routing and it seems to work well in my case. I've read the thread you've linked and saw that you were discussing a similar thing with the maintainers of Carbon Components. Perhaps we should polish it up and publish it, maybe even under the hood of the Svelte organization. What do you think?

As a future improvement, we could make this automatic – follow the Node resolution algorithm and rewrite imports without specifying the rule.

@benmccann
Copy link
Author

Yes, I proposed making a generic library for it in carbon-design-system/carbon-preprocess-svelte#20. Though there's also been another proposed solution that we may want to consider: sveltejs/kit#2612 (comment)

It seems like the PR to convert to SvelteKit is getting pretty close now! Is there anything else you want to fix before merging it?

@illright
Copy link
Owner

First we got to set up the static exports and verify that everything works. Shouldn't be too long

@benmccann
Copy link
Author

benmccann commented Oct 17, 2021

Oh, yeah, I forgot about that. We will have to wait a little for that because we'll need lgarron/clipboard-polyfill#144 fixed before adapter-static will work. Unless you want to add a script that adds "type": "module" to node_modules/clipboard-polyfill/package.json as a temporary patch, but we can probably just wait since the author of that repo seems to be pretty responsive

@illright
Copy link
Owner

illright commented Oct 17, 2021

pnpm has this dope feature called packageExtensions where it lets you inject fields into the package.json of your dependencies. Wonder if that could work as a temporary solution.

UPD: never mind, it can't. I've just read the docs a bit further, and it says that. Oh well.

@illright
Copy link
Owner

illright commented Oct 18, 2021

I've added the static adapter. There seems to be an issue regarding the link paths in the resulting HTML.

I run the following command (after patching up clipboard-polyfill's package.json):

attractions/docs$ env APP_BASEPATH=/attractions/1337 pnpm build

It generates an index.html of the following fashion:

<head>
  <!-- hardcoded in app.html -->
  <link rel="manifest" href="/manifest.json" crossorigin="use-credentials" />
  <meta name="msapplication-config" content="/browserconfig.xml" />
  
  <!-- generated in place of %sveltekit.head% -->
  <link rel="modulepreload" href="/attractions/1337/_app/start-8730cdc3.js">
</head>

Notice how the path of "modulepreload", which was generated in place of %sveltekit.head% is rebased onto /attractions/1337, but the URLs that were already there (in app.html) are still based on root. This difference makes it impossible to fix the paths with the <base> tag.

Sapper used to have a placeholder %sapper.base%, SvelteKit doesn't seem to have it. How can we work around this?

Another thing that isn't great about non-standard bases with SvelteKit: when you run svelte-kit preview, it hosts the application on /, ignoring the setting of the base path. Additionally, the directory structure of build is not changed whenever the base is different. In Sapper we had __sapper__/export/..., where the directory structure obeyed the SAPPER_APP_BASEPATH, which made it possible to do something like python -m http.server inside __sapper__/export and see the application exactly as it's gonna be in GitHub Pages.

@benmccann
Copy link
Author

Since 1.0.0-next.185, you can set experimental.prebundleSvelteLibraries: true in svelte.config.js to test out the new prebundling handling to solve the speed issue without the need for a preprocessor. 1.0.0-next.185 also allows you to use Vite's define and environment variables in .svelte templates

@benmccann
Copy link
Author

Sapper used to have a placeholder %sapper.base%, SvelteKit doesn't seem to have it. How can we work around this?

You could file a feature request in the SvelteKit repo and in the meantime you could create a src/hooks.js file that uses a handle method to search and replace those strings to include the base path

@benmccann
Copy link
Author

import.meta.url seems to work correctly. The problem is in how it is used, and that is with the new URL constructor. The returned url is invalid on Windows since it starts with a forward slash.

I haven't tested, but possibly that will be fixed when we upgrade to the latest version of Vite vitejs/vite#5268

@illright
Copy link
Owner

in the meantime you could create a src/hooks.js file that uses a handle method to search and replace those strings to include the base path

But that still doesn't solve the issue of some paths being static and others (ones generated by SvelteKit) being rebased onto the base path. Any idea on how to prevent SvelteKit from generating links with the base of /?

@benmccann
Copy link
Author

You could prefix the static links with some placeholder like %svelte.base% and then replace it in handle.

@illright
Copy link
Owner

Is this a known issue though, or should it be filed as a bug?

@benmccann
Copy link
Author

I don't think there's any open issue to track it

@illright
Copy link
Owner

There is now. We'll wait until this is resolved to continue migrating.

@benmccann
Copy link
Author

It seems a nice thing for us to add, but it's relatively low priority given that it doesn't affect all users and can be implemented on the user-side. I'd consider a PR if you want to send one or you can add the workaround here. I just wanted to be honest with you, that I think you'd be waiting quite a long time if you waited for us to implement it.

@benmccann
Copy link
Author

sveltejs/kit#2697 has been resolved, so hopefully you can continue the migration now!

@illright
Copy link
Owner

@benmccann We've proceeded further with the migration, however, now, a rather puzzling error appears upon trying to export the site:

> Using @sveltejs/adapter-static
> 404 /docs/components (linked from /docs/components/autocomplete-option)
    at file:///home/illright/work/attractions/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.230_sass@1.48.0+svelte@3.46.2/node_modules/@sveltejs/kit/dist/chunks/index5.js:314:11
    at visit (file:///home/illright/work/attractions/node_modules/.pnpm/@sveltejs+kit@1.0.0-next.230_sass@1.48.0+svelte@3.46.2/node_modules/@sveltejs/kit/dist/chunks/index5.js:473:5)

To reproduce this, run pnpm docs:build in the root directory of the repository.

The error suggests that we have a link to /docs/components somewhere in /docs/components/autocomplete-option, however, there is none. I've checked the source code, the "View Source" view in dev mode, the produced files in .svelte-kit and build directories, none of them contain a link to /docs/components, only to /docs/components/<whatever>, which exist in the routes. I don't quite understand where this link comes from. Any ideas?

@benmccann
Copy link
Author

This is a regression in Kit. I sent a fix (sveltejs/kit#3367). In the meantime, you can make it work by temporarily downgrading to 1.0.0-next.220

@benmccann
Copy link
Author

The SvelteKit fix has been merged and release, so it should work now with the latest version of SvelteKit

@benmccann
Copy link
Author

@illright just checking in on this since it seemed like you were almost done with it. is there anything you're currently stuck on that I might be able to lend some advice about?

@illright
Copy link
Owner

Nothing particular that we're stuck with, we're not making much progress due to a lack of motivation :) I appreciate you checking in though, I might come back to this for another burst some time soon.

@aabounegm aabounegm linked a pull request Jul 5, 2022 that will close this issue
14 tasks
@aabounegm
Copy link
Collaborator

aabounegm commented Jul 8, 2022

@benmccann I have restarted the work in the new Pull Request linked above. Please take a look and tell me what you think. The failing pipeline for now is the linting one, which is expected when moving from JavaScript to TypeScript and uncovering a lot of problems. but there is one issue I could use help with: svelte-check (and the VS Code extension as well) reports this problem: Cannot find module '$app/paths' or its corresponding type declarations. (actually for any $app/... import). Any idea how to fix that? (nvm, fixed it)

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 a pull request may close this issue.

3 participants