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

chore: upgrade to zeit 2 #998

Merged
merged 1 commit into from
Feb 16, 2019
Merged

chore: upgrade to zeit 2 #998

merged 1 commit into from
Feb 16, 2019

Conversation

nolanlawson
Copy link
Owner

More work on fixing #985.

I'm not a huge fan of this implementation, because we now basically have two different "production" implemenations - one using node server.js (using Express + Sapper middleware), and another using Sapper export plus a static fileserver in Now.

But it does work, and the difference between the two is minimal. The only thing I had to do slightly differently was handling dynamic URLs like /accounts/<accountId> - instead of letting Sapper middleware handle it, I just return service-worker-index.html. This is fine, because literally the only case where this matters is if a user is on one of those dynamic pages, and either 1) hard-refreshes, and 2) doesn't have a Service Worker, and refreshes. Those cases are so rare that the difference doesn't matter much.

More troubling is having to manually manage this entire CSP mess, as well as a big regex to identify the dynamic URLs. But since we have a dev server to catch any mistakes I might make, I'm not too concerned. But it could be improved.

@nolanlawson
Copy link
Owner Author

Also once sveltejs/sapper#566 is merged into Sapper, we can potentially just use sapper export as the "normal" build process. The reason I don't want to do that now is because without that PR, Sapper would always use port 3000 for exporting, which may conflict with self-hosters who might be running something else on 3000, e.g. Mastodon.

@nolanlawson
Copy link
Owner Author

Just occurred to me that we will lose the link rel=preload in the header due to this PR, but I think it's worth it to get the CDN working and switch to a faster static model. Even without the CDN, this seems faster since it doesn't have to go through a Node server.

@nolanlawson nolanlawson merged commit 0d9dc78 into master Feb 16, 2019
nolanlawson added a commit that referenced this pull request Feb 16, 2019
As of #998 we can no longer rely on JS files containing , let alone . This was a nice idea, but it's just not maintainable anymore nor does it provide a huge benefit.
nolanlawson added a commit that referenced this pull request Feb 16, 2019
As of #998 we can no longer rely on JS files containing , let alone . This was a nice idea, but it's just not maintainable anymore nor does it provide a huge benefit.
@Rich-Harris
Copy link

Just occurred to me that we will lose the link rel=preload in the header due to this PR

keep an eye on this issue 😀 sveltejs/sapper#402

@Rich-Harris
Copy link

oh man, I just saw sveltejs/sapper#568 🤦‍♂️

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.

2 participants