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

Exporting #88

Merged
merged 25 commits into from
Jan 15, 2018
Merged

Exporting #88

merged 25 commits into from
Jan 15, 2018

Conversation

Rich-Harris
Copy link
Member

This builds on top of @freedman's PR #66. The main difference is that we're just monkey-patching fetch to reify server routes like /api/blog/whatever rather than making any assumptions about project structure or adding configuration.

Rather than running everything through node-spider, we just copy assets and the generated files, and then visit / (TODO: visit all static routes.)

This is quite a crude approach but it works well with the test app. Lots of tasks remaining:

  • monkey-patch XHR as well
  • handle relative URLs
  • handle URLs with query strings and hashes
  • more logging

Also, semi-relatedly, the CLI will need a bit of work (sapper --help etc — also, would be nice to configure the dist folder perhaps).

I think we might be better off using sapper export than sapper extract, so that people familiar with the concept from Next.js know what to expect — what do you reckon? Or sapper generate, to align with Nuxt.js.

@freedmand
Copy link
Contributor

freedmand commented Jan 15, 2018

Much cleaner! I was never too comfortable using node-spider and actually parsing HTML pages.

I just tried this on a personal site I am building that's closely coupled in style to the template project. I'm getting this error:

ERROR in ./templates/.main.rendered.js
Module not found: Error: Can't resolve 'sapper/runtime.js' in '[full_project_path]/templates'

Any immediate thoughts? (This is after 14 other files seem to be successfully built)

@Rich-Harris
Copy link
Member Author

That's very odd — does [full_project_path]/node_modules/sapper/runtime.js exist?

@freedmand
Copy link
Contributor

No, it doesn't, but I must say I'm relatively new to debugging node packages with GitHub and am sure I installed this commit wrong. Sorry to pollute this thread, but what's the best practice for pulling this change in inside an actual Sapper project?

I went into the node_modules directory, removed sapper and then used degit for your latest commit and tried to install it. This seems like it is bound to be extremely error-prone.

@freedmand
Copy link
Contributor

freedmand commented Jan 15, 2018

I changed the dependency in my package.json for my actual Sapper site to "sapper": "git://github.com/sveltejs/sapper.git#fe03fd3a5293289e2c31ef229fa4f95d1b9d03c7", where the really long hash is -- at the time of writing -- the latest commit. After running npm install, it still shows no sapper/runtime.js. I'm guessing something is happening that's not compiling runtime.js for some reason?

@Rich-Harris
Copy link
Member Author

Ah, I see — yeah, that would fail. The runtime.js file isn't checked into the repo because it's generated.

The way I'm doing it is by running npm run build && yarn link inside sapper, and yarn link sapper inside the project I'm testing. (yarn link instead of npm link because it seems much more reliable.)

@freedmand
Copy link
Contributor

freedmand commented Jan 15, 2018

EDIT: I think it may be creating a file with the same name as a directory, and that's preventing further files in that subdirectory from being created.

Yep, just got it working by manually merging changes from this commit.

Now I'm facing new issues. It seems not all routes are generated in my project -- I'm getting some 404s in my website project. [slug].html routes are not being generated. Also the extract command returns these errors, which may be non-harmful:

(node:9477) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: EEXIST: file already exists, mkdir '[project_dir]/dist/api/projects'

@freedmand
Copy link
Contributor

Any thoughts on files that have the same name as directories? It's cool in servers but not most filesystems.

Ex:

Let's say I have the following routes:

  • api/blog (JSON file listing blog posts)
  • api/blog/post1
  • api/blog/post2

Generating these files locally results in errors because blog is both a file and directory within api. Possible solution would be adding explicit extensions, but this may mess up other routing mechanics or make it less clean for the non-generated version.

@freedmand
Copy link
Contributor

This is easily fixable by adding /index.html to dest within fetch, as long as it's cool to use an HTML extension for JSON (or other types). It seems to be a fair workaround for now.

@Rich-Harris
Copy link
Member Author

Yeah, I ran into the directories issue as well. For pages adding /index.html seems like it's probably okay. Adding it to API endpoints feels very weird, am try to resist that... will have to look at how Next and Nuxt deal with this, if at all.

It would be easy enough to determine that /api/blog/index.js and /api/blog/[slug].js are incompatible, and throw an error accordingly if an app with conflicting routes is exported. So at least we can make those failures less mysterious. I have an exported build of https://sapper.svelte.technology locally, and I basically just changed https://sapper.svelte.technology/api/guide/contents (which conflicts with https://sapper.svelte.technology/api/guide) to https://sapper.svelte.technology/api/guide-contents.

One other possible idea — probably a terrible one — would be to generate a map that worked like so:

const route_map = {
  '/blog': '/blog_xyz123'
};

const _fetch = window.fetch;
window.fetch = (url, opts) => {
  url = new URL(url, window.location.href);
  if (url.pathname in route_map) return _fetch(route_map[url.pathname], opts);
  return _fetch(url, opts);
};

Glossing over some details (query string parameters etc) but you get the gist.

Now I'm facing new issues. It seems not all routes are generated in my project -- I'm getting some 404s in my website project.

Interesting. Can you get to those pages from /, just by clicking links? The idea is that it follows all the links it finds. Three possibilities:

  • we need to use all static routes as starting points for the search (e.g. /blog as well as /)
  • it's something to do with relative URLs (not sure how robustly it copes with those, compared to absolute URLs)
  • there's a bug

@Rich-Harris Rich-Harris merged commit 0e131cc into master Jan 15, 2018
@Rich-Harris Rich-Harris deleted the gh-9 branch January 15, 2018 15:24
@Rich-Harris
Copy link
Member Author

This feature isn't done, but I'm going to release it as 'experimental' in 0.5 so that we can start properly dogfooding, and iterating from there. Many thanks @freedmand for outlining the solution and getting the ball rolling

@freedmand freedmand mentioned this pull request 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.

2 participants