-
-
Notifications
You must be signed in to change notification settings - Fork 434
Conversation
src = locations.src(), | ||
dest = locations.dest(), | ||
routes = locations.routes(), | ||
cwd = process.cwd(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it better to do path.resolve(opts.cwd || '.')
then path.join
directories from there. It'll arrive at the same value most of the time, but allows dev to specify a --cwd foobar
flag, which is useful for testing or some power-user shiz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it amount to the same thing currently? If cwd
is specified (as an option, not a CLI flag, which isn't supported just yet... TK) then it uses that, otherwise it uses process.cwd()
. It's how the tests in this PR work (await build({ cwd: __dirname })
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right right, I was thinking towards a flag.. I left my comment in wrong place.
The flag would be nice as it means one can use something other than the SAPPER_*
global to set the cwd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we discard SAPPER_BASE
etc altogether? With the new API they're not really pulling their weight, and they're not documented anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I guess if we wanted to be rigorous we'd have flags for all the various path options — --src
, --routes
, one for the __sapper__
directory (--output
?) that's separate from the --build-dir
option... maybe it should be a follow-up PR now that I think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmm, I'll defer to you on that. I think as long as the --cwd
flag makes it in (or similar), it'll be okay.
I may also be biased since I prefer relying on flags or options rather than globals. As long as they're all removed (or all kept), it won't be confusing.
I've only seen the BASE/ASSETS globals used in deploys to Lambda or GCP Functions. Every other is internal-facing only iirc.
first = false; | ||
} | ||
|
||
// TODO clear screen? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I would soft-clear here. I've found that whenever I do a hard clear I get some push back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know about console-clear — handy, thanks. This is an old TODO though (I just copied it from a now-deleted file), should probably be dealt with in a separate PR
This improves the JavaScript API somewhat — it makes it promise-based, and less reliant on magic env vars. No real external benefit, just better code organisation.
TODO:
sapper start
andsapper upgrade
from the CLI, since they're unmaintained, untested and currently do anything, and no-one has noticed.process.chdir()
andprocess.env.NODE_ENV = 'production'
stuff from the tests, since this should no longer be necessary