search

thgh / vercel-sapper

Vercel builder for Sapper with SSR enabled
MIT License
189 stars 25 forks source link

Feature/memory config #44

Closed antony closed 3 years ago

antony commented 4 years ago

Fixes https://github.com/thgh/vercel-sapper/issues/33 in a way which means we don't need to maintain our own version of the template.

Fixes https://github.com/thgh/vercel-sapper/issues/43 by allowing memory configuration

Fixes https://github.com/thgh/vercel-sapper/issues/39 and https://github.com/thgh/vercel-sapper/issues/38 because they're invalid - the readme, examples, and this PR work without issue.

back to 0 issues!

thgh commented 4 years ago

Do you mind not closing unrelated issues with this PR? I wish to keep issues open as a signal that something may be unstable and to lower the barrier for possible contributors that may comment with reproduction info. I know that there are quite some similar unreproducible open issues, but I hope we can improve the project so it's impossible to run into issues. #pitofsuccess

antony commented 3 years ago

@thgh which unrelated issues does this refer to? The two "does not work issues"? Both of them claim that checking out the template and trying to run it doesn't work, but it's user error, since this PR proves that it does.

I don't see the value in keeping misleading issues open when there are unknown environmental concerns/user oversights which might affect them - and we can prove, reproducibly, that the provides steps do work. It's offputting to potential users. Both issues have also asked for clarification over a month ago, and had no response, so they've become stale / were indeed errors, and were silently fixed by the authors.

I feel like it's off-putting to future users when there are open issues which seem to imply that the project simply doesn't work. However, it's up to you.

antony commented 3 years ago

Hey, this is pretty cool! I have been tinkering how we could make using vercel-sapper easier and this seems like a good start. I'm not sure though if this is easier than manually addng the files and code. I imagine the perfect experience (apart from built-in support) would be to run npx vercel-sapper on an existing sapper project. What do you think?

In my opinion, being able to run npx vercel-sapper on a project is problematic for two reasons:

1) The script to do this would be intolerant of any changes to server.js, and unpredictable at best. I certainly wouldn't run such a script, and it would definitely break my projects. 2) Making the builder also a cli which installs the builder, is pretty confusing.

It's a nice idea, but I'm certainly of the opinion that manual control is better.

A deploy to vercel button might be a better idea.

thgh commented 3 years ago

This is the result: https://github.com/thgh/vercel-sapper/pull/47

  1. It should be idempotent and careful not to break projects. It probably will break some projects, but hopefully those edge cases can be ironed out. As for trusting to run a script, everyone can always inspect it.
  2. True, feel free to create a vercel-sapper-template project, but for maintainance purposes, I will include it here so all feedback comes in one place. Could be separated at some point.