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

move app logic into templates (#444) #453

Merged
merged 8 commits into from
Oct 1, 2018
Merged

move app logic into templates (#444) #453

merged 8 commits into from
Oct 1, 2018

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Sep 30, 2018

This lays the foundation for #444, by moving app logic out of the Sapper package (sapper/dist/middleware.js and sapper/runtime.js) and into the generated files.

Those generated files, which used to be project/src/manifest/{client,server,service-worker}.js are now written to the project/src/__sapper__ directory. That is still up for debate. One possibility is to put all generated files in project/__sapper__ (i.e. outside src), so that it would look like this:

├ __sapper__
│ ├ .build
│ │ ├ dev
│ │ │ ├ # dev files here (during `sapper dev`)
│ │ ├ prod
│ │ │ ├ # prod files here (during `sapper export`)
│ ├ client.js
│ ├ server.js
│ ├ service-worker.js
├ build (output of `sapper build` — should this be `dist` instead?)
├ export (output of `sapper export` via `__sapper__/.build/prod`)
├ src
│ ├ # ...
├ static
│ ├ # ...

TODO

  • settle the folder structure question
  • probably need to provide some more informative errors
  • update docs

@lukeed
Copy link
Member

lukeed commented Sep 30, 2018

I think anything generated should live outside the working files entirely. I can totally go either way on this, but that'd still be my vote.

The latest structure has src/manifest/*, which is all generated, as well as .sapper/* files. It's trivial for Rollup/Webpack to maintain correct file aliases across directories so that shouldn't be involved in the decision IMO.

This is especially true if we rename the manifest imports to something like this:

import { manifest } from 'sapper/server';
//
import { manifest } from 'sapper/client';
//
import { timestamp, assets, shell, routes } from 'sapper/sw';

// or, of course, use 1 file and export manifests/partials from it

import { server as manifest } from 'sapper/manifest';

@lukeed
Copy link
Member

lukeed commented Sep 30, 2018

Ah, I forgot this issue drops the manifests entirely 😇

Either way, I'd still vote for having all generated files in a single directory, preferably away from my user-source files.

@Rich-Harris
Copy link
Member Author

To clarify, is that a vote for the structure shown in the tree diagram above?

Relatedly, should project/build become project/dist?

@lukeed
Copy link
Member

lukeed commented Sep 30, 2018

Sorta. I think this makes the most sense:

├ __sapper__
│ ├ dev
│ │ ├ # dev files here (during `sapper dev`)
│ ├ build
│ │ ├ # prod files here (during `sapper build`)
│ ├ export
│ │ ├ # prod files here (during `sapper export`)
│ ├ client.js
│ ├ server.js
│ ├ service-worker.js
│
├ src
│ ├ # ...
├ static
│ ├ # ...

@Rich-Harris
Copy link
Member Author

Oh, interesting. I can definitely see the appeal of that. The only downside is that node build becomes node __sapper__/build (to run the built app) and instead of npx serve export to serve an exported app we have npx serve __sapper__/export. But that doesn't feel like a dealbreaker.

@lukeed
Copy link
Member

lukeed commented Sep 30, 2018

I would go further and rename the directory to sapper and rename server.js to index.js, so that running a server becomes node sapper and running the exports becomes npx sirv sapper/export

@Rich-Harris
Copy link
Member Author

I think it needs to be something like __sapper__ rather than sapper for the same reasons src/manifest doesn't quite work — it's not totally obvious that it contains generated files, and it's likely to sit between the user's directories (e.g. the svelte.technology project has a content directory and a src directory, and it's weird if sapper goes between them).

Also, __sapper__/server.js isn't executable, it's just something that src/server.js imports. The file written to build is already called index.js — that's how node build works.

(Btw the reason I say npx serve export instead of npx sirv export is because I can never be bothered to look up how to invoke the binary of a package that doesn't share its name. I don't suppose you've ever considered merging sirv and sirv-cli...?)

@lukeed
Copy link
Member

lukeed commented Sep 30, 2018

RE __sapper__: totally understood, I agree which is why I didn't include the suggestion originally.

RE server.js: gotcha – made bad assumption that that had changed as well. Sounds good.

RE sirv: Lol thew that in there to see if you'd notice 😆 It doesn't matter to me that you use it! I get the frustration, but it's to avoid downloading the CLI counterpart when you're just after the server module. While you and this circle are keen on tree-shaking all the way down the line, the 99% aren't or won't; so I've solved/avoided that problem thru separation of concerns rather than changing their toolchains & habits.

@Rich-Harris
Copy link
Member Author

Ok, I've put everything in Sapper. It is nice and neat. Will try this branch out with a few different projects and see if I've missed anything.

No-one's apps would be adversely affected by having sirv-cli inside sirv, tree-shaking or not — it'd just mean having to install a tiny handful of additional dependencies that don't end up getting used. Personally I'd say it's well worth it for the ease of doing npx sirv . wherever you are. But that's just me 😀

@Rich-Harris Rich-Harris changed the title [WIP] move app logic into templates (#444) move app logic into templates (#444) Sep 30, 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