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

Move sirv-cli to dev dependencies #57

Merged
merged 1 commit into from
Nov 15, 2019
Merged

Conversation

Crisfole
Copy link
Contributor

You almost certainly didn't mean for sirv-cli to wind up in the dependencies list.

You _almost certainly_ didn't mean for sirv-cli to wind up in the dependencies list.
@sanderhahn
Copy link

The dev dependencies will be pruned and that makes npm start fail: #44

@Crisfole
Copy link
Contributor Author

Hm...I think I disagree on using sirv in production to serve up purely static files...But it sounds like that ship has sailed. In case it has not...

Changes like these belong as instructions in the README, not in the default configuration. New programmers will see sirv-cli in the dependencies and it will confuse them.

Ideally apps like these should be built and tested on a CI server (or locally for a slightly simpler workflow) and then uploaded (with or without sourcemaps) to a server optimized for serving static files...

All that said, I can be convinced I'm wrong, so please explain if I am - I always like to learn.

@subz390
Copy link

subz390 commented Aug 22, 2019

I'm confused about this too - do you have serv in the dependencies because you expect everyone to normally be serving their web app with it? What about on services like Netfly and ZEIT Now? can I move it myself into devDependencies without issue, will it change the compiled package so I cannot use it anymore? Thank you.

@Rich-Harris Rich-Harris merged commit 8003b25 into sveltejs:master Nov 15, 2019
@Rich-Harris
Copy link
Member

Gah, I've gone back and forth on it a few times, and had concluded that sirv-cli does belong in devDependencies — you really should be deploying static files if possible. But I can't figure out how you're supposed to do that in the Heroku case in particular. So I'm going to revert this :(

@lukeed lukeed mentioned this pull request Nov 25, 2019
@mvllow
Copy link
Contributor

mvllow commented Nov 25, 2019

Possibly a naïve solution (that I can't test atm) but could we do npm i -P sirv-cli && sirv public or similar if deploying? This should at least move sirv-cli from dev to prod deps.

Edit: Maybe this? With better naming of course 🤔

Leave "start": "sirv public" but add:

"start:nonstatic": "npm i -P sirv-cli && sirv public"

@guidobouman
Copy link

guidobouman commented Dec 9, 2020

Let me echo my previous comment: What is the responsibility of this template?

To get up and running with your project in a semantic way, or to run a server that serves files when deploying to production? That last one sounds more like Sapper's responsibility to me.


EDIT: To summarise, this template should not worry about running on servers. To let Heroku run a node script (that is meant for development) to serve static files in production feels like a very bad code smell to me.

@antony
Copy link
Member

antony commented Dec 9, 2020

To get up and running with your project in a semantic way, or to run a server that serves files when deploying to production? That last one sounds more like Sapper's responsibility to me.

It doesn't have anything to do with Sapper, that's a different project.

I would say that this template is a basic, here's how you could get up and running in a way that is easy to understand template. If you're deploying a production site from this template then it's your responsibility to ensure that the architecture is sufficient for your needs.

@guidobouman
Copy link

guidobouman commented Dec 9, 2020

@antony We're saying the same thing here. Sapper is a different project, with its own template. This template should not be worried about the possible deployment environments. I've clarified my previous comment above.

@lukeed
Copy link
Member

lukeed commented Dec 9, 2020

We've had this debate in every form imaginable. Nothing hasn't been said. The template is in a manner that satisfies a larger majority of cases by default.

Luckily, it's as easy as moving - or deleting - 1 line if you're not in the same camp.

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.

8 participants