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

Move error handling into pages #91

Closed
Rich-Harris opened this issue Jan 17, 2018 · 11 comments
Closed

Move error handling into pages #91

Rich-Harris opened this issue Jan 17, 2018 · 11 comments

Comments

@Rich-Harris
Copy link
Member

...and out of templates. Great idea from @mrkishi — if our routes directory had 4xx.html and 5xx.html (or more specific codes, not that we're currently set up to use them) files, then we could use those in place of the current templates/xxx.html files. This has a number of advantages:

  • No duplication — we can just have a single templates/page.html
  • Those pages can themselves use components
  • It gives us a sensible way to handle client-side navigation errors, rather than, err... discarding them which is what we currently do (yeah I know)

It would also simplify the internals slightly.

This was referenced Jan 17, 2018
@Rich-Harris
Copy link
Member Author

Rich-Harris commented Feb 10, 2018

A related thought. Currently, server.js is a CommonJS module, and the Sapper middleware loads the 'server app' generated from the contents of routes via the webpack config.

It turns out that's not ideal for a few reasons. Firstly, it's slightly confusing that server.js and templates/main.js are completely different beasts with different rules. Secondly, changes to server.js don't cause a server restart, because it's exempt from the filewatching mechanism. Thirdly, it adds a lot of complexity around (for example) interacting with user data generated by things like Passport — I've found myself throwing stuff onto the req object just to pass it into my server routes, because my server.js and my individual routes can't access shared modules for managing stuff in a more intelligent way. (My example is probably extremely specific, but I think it hints at a more general issue.)

So I'd like to experiment with a different, simpler structure:

├ package.json
├ webpack.client.config.js
├ webpack.server.config.js
├ app
│ ├ client.js
│ ├ server.js
│ ├ service-worker.js
│ └ template.html
├ assets
│ ├ # your files here
├ routes
│ ├ # your routes here

app/server.js would be able to import and interact with the same modules as routes/**/*.[html|js].

I think that would be a little more intuitive than the status quo, and might yield benefits in terms of startup performance (for Lambda-type environments). There'd be some gotchas to work through though, e.g. we don't want to accidentally bundle chokidar in prod mode. And it's still not clear what we'd do about service-worker.js — does that get processed by webpack? We probably don't want a third webpack config...

@ansarizafar
Copy link

Nextjs is now has Universal Webpack feature https://zeit.co/blog/next5. I think we can also simplify webpack configuration by using a similar feature.

@LorbusChris
Copy link

It would be great if some sort of configurability/composability would be given to the service worker. One might want to extend it with custom behaviour/code/functions. Maybe this could be limited to es6+ and then be done with rollup (and a simple config thereof)?

@Rich-Harris Rich-Harris mentioned this issue Feb 13, 2018
@Rich-Harris
Copy link
Member Author

Been thinking about this a little further. I think we probably should have a separate service worker webpack config (which would be optional, since not everyone wants a service worker), in which case it'd make sense to have a structure like this:

├ package.json
├ app
│ ├ client.js
│ ├ server.js
│ ├ service-worker.js
│ └ template.html
├ assets
│ ├ # your files here
├ routes
│ ├ # your routes here
├ webpack
│ ├client.config.js
│ ├server.config.js
│ └service-worker.config.js

Also, I'd like to get rid of the magically-injected __routes__ variables (and equivalents in service-worker.js). We could simplify everything by importing those values from generated files, and — adhering to Sapper's philosophy of 'don't hide the plumbing' — those generated files could be visible in the project folder.

├ package.json
├ app
│ ├ generated
│ │ ├ client.js
│ │ ├ server.js
│ │ ├ service-worker.js
│ ├ client.js
│ ├ server.js
│ ├ service-worker.js
│ └ template.html
├ assets
│ ├ # your files here
├ routes
│ ├ # your routes here
├ webpack
│ ├ client.config.js
│ ├ server.config.js
│ └ service-worker.config.js

Not totally happy with the naming of generated/client.js, generated/server.js and generated/service-worker.js, but that's open to bikeshedding. I think the idea is sound and I'm going to start working on it.

@arxpoetica
Copy link
Member

@Rich-Harris I know with SSR the client and the server have the appearance of being the same thing, but I generally like a separation of files/concerns when it comes to back end and front end. So often in my folder structures I'll have something like:

├ client
│ ├ app
│ │ ├ # all client source files here
│ ├ public
│ │ ├ # your files here
├ server
│ ├ # anything node/server related here

Otherwise you end up with Rails-like structures of massive folder lists in the main root. Just a thought.

Arguably public or assets or whatever could/should (?) still be top level.

@ansarizafar
Copy link

I personally don't like directory based routing. It would be great If we can define routes with code. Workbox https://developers.google.com/web/tools/workbox/ can be used for service worker generation.

@arxpoetica
Copy link
Member

@ansarizafar Routing is a little off topic for this thread ;) but I think the conversation about this in the past has mainly revolved around allowing both to be possible. For new developers who don't understand the intricacies of coded routing (and lets be honest, they are sometimes difficult to code), out-of-the-box page-based routing can be a beautiful thing. I favor keeping it, but having the ability to add on with more complex routing if so desired.

@thgh
Copy link
Contributor

thgh commented Feb 15, 2018

I like the idea of having the generated files available for insight. I was however confused by the naming. I first expected generated/client.js to be the transpiled version of client.js.

What about collapsing the generated folder into a file?

├ app
│ ├ _generated.js
│ ├ client.js
│ ├ server.js
│ ├ service-worker.js

_generated.js would then contain everything and because of treeshaking we don't have to worry about importing too much.

export const routes = ...
export const assets = ...

Should client/server/sw get different generated data?

(Alternative name propsal: sapper-manifest.js)

@Rich-Harris
Copy link
Member Author

@arxpoetica I can see the appeal of that. Main reason I've avoided it thus far is a) I'm likely to have some (non-route-specific) code that is shared between server and client, and I don't want to have a third universal folder and things like this...

import someutil from '../universal/someutil';

...and b) the [client|server|service-worker].js files are often quite small. But I definitely get where you're coming from. Lemme think on that. (Also, it might turn out that we can make that entirely a matter of choice.)

@ansarizafar

I personally don't like directory based routing

Then Sapper might not be the framework for you!

@thgh I really like the 'manifest' name, that's much better than 'generated' 👍 — thank you. I considered putting everything in a single file. The routes export is different in each case though. In the client, it's an array of objects with pattern, params, and a load function that triggers the import(); on the server, load is replaced with module (no lazy loading), and the array includes server routes as well as pages; in the service worker, it's just pattern. Exporting all three things from a single file might be a bit confusing.

Rich-Harris added a commit that referenced this issue Feb 18, 2018
[WIP] restructure everything
@arxpoetica
Copy link
Member

@Rich-Harris I see your point. One problem that a lot of people have (for example) with isomorphic apps is this kind of blurring of the lines. Maybe the right thing to do then (against my own better judgement, ha) is to thoroughly or deliberately embrace that blurring, i.e., Sapper is truly trying to straddle that line because it does it really, really well. I admit I'm not 100% convinced of this argument, but it seems worthy/aspirational.

Of note, Meteor tried to straddle this line and had oodles of lovers and haters. But it also had the problem of boxing people in to strict convention. Sapper ("lovingly") tries to step out of the way and only has a few "right" opinions, lol. For example, on my end I already have a client and server folder as needed and it doesn't harm Sapper in any way and it doesn't harm me, so things seem to be working just fine.

I mostly just need to wrap my brain around it all. Like you said, my brain is going to be thinking on it too, but I think it's perfectly fine as you have it above, i.e., all under app.

@Rich-Harris
Copy link
Member Author

This stuff was all implemented in 0.7, so this can be closed. Re project structure, it should be fairly straightforward to make it entirely configurable, though for now it expects the structure laid out above (since configurability has its own costs, in that it adds to the list of things developers have to worry about)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants