Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Adds a sapper extract CLI command, which scrapes the server to run … #66

Merged
merged 12 commits into from
Jan 15, 2018

Conversation

freedmand
Copy link
Contributor

@freedmand freedmand commented Jan 5, 2018

…as a static website starting at the site's root. Resolves #9

TESTED=Basic unit test ensuring relevant routes are added.

…as a static website starting at the site's root.

TESTED=Basic unit test ensuring relevant routes are added.
* @param {number=} extractionDir The directory in which to place the extracted
* output.
*/
module.exports = async function(includeUrls = null, excludeUrls = null,
Copy link
Member

Choose a reason for hiding this comment

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

Super cool stuff! There is an issue to move away from async/await. Maybe it's best to preemptively not use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Hmm, there are a lot of await calls in Sapper already. I'm happy to change out my await and async calls, but do you think the onus of not being able to run on platforms like AWS Lambda because of the Node version is on the platforms to fix?

await_in_sapper

Copy link
Member

Choose a reason for hiding this comment

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

@Rich-Harris has more insight on that than me, but I can definitely see the appeal of supporting lower versions of Node.
I just have all the async/await in test.js left to replace in a PR, so no worries!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed async/await in my changes to keep you from having to do the work :)

'api/blog/how-can-i-get-involved/index.html',

'blog/how-is-sapper-different-from-next/index.html',
'api/blog/how-is-sapper-different-from-next/index.html',
Copy link
Member

Choose a reason for hiding this comment

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

A little funky indentation.

@freedmand
Copy link
Contributor Author

freedmand commented Jan 5, 2018

Just realized this is currently not producing a navigable website in the test/app directory. Working to fix now

EDIT: fixed and added unit test clauses to cover it

@lukeed
Copy link
Member

lukeed commented Jan 6, 2018

FWIW, my PR fails within Travis the same way & it didn't change anything in relation to that test. Maybe it's off?

@freedmand
Copy link
Contributor Author

If you look back in the pull request history, Travis is mostly failing, so I agree something seems off. I'm confident my PR doesn't contribute new failures.

(On that note, there seems to be some amount of non-deterministic behavior in the tests. When my computer processor was overloaded, some of the tests sporadically failed, maybe since they're based on timeouts?)

@Rich-Harris
Copy link
Member

Thanks very much for this, and apologies for leaving it unattended a while.

I'm actually having a few issues getting it to work correctly — if you serve the test/app/dist directory and click around, it fails to navigate. Possibly a few things going on here:

  • Changing /foo to /foo/, backed by /foo/index.html, might be confusing the router
  • Things like /api/blog/what-is-sapper (which contains JSON data) is being written as /api/blog/what-is-sapper/index.html which is incorrect

Will keep chipping away at it (I added some commits to this PR already, mostly so that it doesn't try to extract without first building the site in prod mode), but noting it in here in case it you already know the answer 😀

@freedmand
Copy link
Contributor Author

I'll have time to look into it more in a few hours. I suspect it may depend on the static router. I think python -m SimpleHTTPServer and Netlify were cool redirecting things automatically so that it would work. Are you using serve or something else (I haven't looked into other things)?

@Rich-Harris
Copy link
Member

I'm using serve, yeah. FYI I'm investigating a slightly different approach that involves using the server's entry point and generating the pages without spinning up a server — early results are promising, and it should be blazing fast!

@lukeed
Copy link
Member

lukeed commented Jan 14, 2018

^ I'm working on that locally. Looks good but I think have some stuff missing. The labor of being 3 months out of date with Svelte

@freedmand
Copy link
Contributor Author

That would be great. There is no reason to spin up a server (while researching this I was shocked there's no non-hacky way to get Express to feed you pages without consuming a port). For this PR, I figured getting a messy approach first that can be iteratively improved would be a good starting point.

Still need to figure a good solution for having the extract pipeline know about server routes that are called in JS without any extra config options (like the /api/ routes in the example project) -- does anyone have thoughts on that?

@Rich-Harris
Copy link
Member

I actually rescind my earlier comments... server routes are a real challenge if you don't have, well, a server. My current line of attack is to replace global.fetch with a function that intercepts calls to e.g. /api/blog/whatever and knows that it needs to save the result. I think we can do it without adding any additional config.

@freedmand
Copy link
Contributor Author

Just to be clear, would the server actually run frontend JS code to figure out what it needs? Let me know if I can help

@Rich-Harris Rich-Harris mentioned this pull request Jan 14, 2018
4 tasks
@Rich-Harris
Copy link
Member

My current thinking is that we wouldn't run client-side code, because that opens a big can of worms. Instead, we rely on pages specifying their data dependencies in preload.

In time, we could probably add a way of configuring additional resources to export, on top of the ones that are automatically discovered.

I opened a new PR on top of this one, since that was the easiest way to make additional changes — #88

@Rich-Harris Rich-Harris merged commit 50011e2 into sveltejs:master Jan 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants