Feature/memory config #44
Conversation
There was a problem hiding this comment.
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?
| @@ -56,7 +57,8 @@ exports.build = async ({ | |||
| ...applicationFiles | |||
| }, | |||
| handler: 'launcher.launcher', | |||
| runtime: runtime | |||
| runtime, | |||
| ...memory ? { memory } : {} | |||
There was a problem hiding this comment.
Why not simply memory : config.memory?
There was a problem hiding this comment.
Are we defaulting it? I need to check.
There was a problem hiding this comment.
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-sapperon an existing sapper project. What do you think?
The reason I don't like this approach is that it requires manual maintenance of dependencies and such as the template is updated. It's the same reason the svelte TS template is a patch rather than yet another unmaintained fork.
There was a problem hiding this comment.
What manual maintenance would be required? I think it would run the same install.js but it saves the user from having to clone this repo, running npm install, cd into the examples directory.
There was a problem hiding this comment.
Who would be responsible for manually merging the original repo into this one every time it is updated?
There was a problem hiding this comment.
No manual merging required, check it out! #47
| To install a working example of a vercel-sapper template ready for deployment or development, see the `examples` directory. | ||
|
|
||
| ``` | ||
| cd examples/sapper-template |
There was a problem hiding this comment.
Is the first step npx degit ...vercel-sapper or git clone ... or how do you see this?
There was a problem hiding this comment.
I could probably include a git clone step before this. I'm not sure degit is required since the template does its own degit, and there isn't really any need to remove git control from the parent project.
|
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 |
|
@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. |
In my opinion, being able to run
It's a nice idea, but I'm certainly of the opinion that manual control is better. A |
|
This is the result: #47
|
Fixes #33 in a way which means we don't need to maintain our own version of the template.
Fixes #43 by allowing memory configuration
Fixes #39 and #38 because they're invalid - the readme, examples, and this PR work without issue.
back to 0 issues!