-
Notifications
You must be signed in to change notification settings - Fork 922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build --watch mode #782
build --watch mode #782
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/3e6arx1mg |
@@ -52,25 +49,203 @@ async function installOptimizedDependencies( | |||
return installResult; | |||
} | |||
|
|||
class BuildPipelineFile { |
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.
Still needs docs, but this is the meat of the refactor: each step of the build was moved from a "full batch effort" to a "single file effort". This way, when a single file was changed we could re-run the entire build on that one file.
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.
Very +1 for this improvement. I’d love for this to be a stepping stone to every file being handled async, and building / watching everything in parallel.
|
||
async buildProxy() { | ||
for (const importProxyFileLoc of this.allImportProxyFiles) { | ||
if (existsSync(importProxyFileLoc)) { |
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.
this is especially hacky, and doesn't really work without --clean
. Will need to be cleaned up
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.
What is the scenario where overwriting is bad (and should it be a comment here)?
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.
already gone :)
Is there an easy-ish way to pull this branch in as a yarn dependency to give it a shot? I'd like to try it out and give some feedback. Not sure what the best approach is to do that though. |
Great call, I just published this PR to |
Mildly unrelated but this version and 2.7.7 seem to be copying over my Anyways, seems to work for me! Obviously a bit noisy right now with the Noticed a problem - if I put a syntax error in my code, then it'll write my Example (missing type for export const Foo = (props: ) => {
return true;
} It also doesn't seem to warn/error on typescript errors, not sure if it should? But it would with webpack for example. |
@francislavoie thanks for the feedback! We're doing some work in #762 which should address those output issues before the next release goes out. @drwpow any idea why those |
8432bd4
to
ffe8cdb
Compare
ffe8cdb
to
69f58a7
Compare
Update: the PR has been cleaned up, and is ready for review! A lot of the PR is moving around existing code, so I tried my best to draw the line between refactoring only what was needed and refactoring every line in the build file. Updated release available at |
69f58a7
to
f88399f
Compare
Latest build still works 👍 Previous comments still apply - Tangential question - is it possible at all to have HMR work with |
filesToResolve: Record<string, SnowpackSourceFile> = {}; | ||
filesToProxy: string[] = []; | ||
|
||
constructor( |
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.
nit: should these be an object instead? I’d be worried about accidentally mixing up a string
type def on a new instantiation
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'm game 👍
* individual stages so that the entire application build process can be tackled | ||
* in stages (build -> resolve -> write to disk). | ||
*/ | ||
class FileBuilder { |
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.
👍🏻
const buildPipelineFile = new FileBuilder(locOnDisk, outDir, buildDirectoryLoc, config); | ||
buildPipelineFiles[locOnDisk] = buildPipelineFile; | ||
await buildPipelineFile.buildFile(); | ||
} |
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.
not important, but could anything be gained here from running the file builders in parallel?
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.
100%, we'd talked about this a bit but if there are no objections I wanted to implement the feature in one PR and then optimize it in another.
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 yeah agreed; don’t mess with child processes in this PR but my comment was specifically: should we replace the for (const …
+ await …
with a Promise.all(…
here?
sourceMaps: config.buildOptions.sourceMaps, | ||
}); | ||
// "--watch" mode - Start watching the file system. | ||
// Defer "chokidar" loading to here, to reduce impact on overall startup time |
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.
👍🏻
packages/snowpack/tsconfig.json
Outdated
@@ -6,8 +6,6 @@ | |||
"esModuleInterop": true, | |||
"strict": true, | |||
"noImplicitAny": false, | |||
"noUnusedLocals": true, | |||
"noUnusedParameters": true, |
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.
Why were these removed?
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.
not meant to be committed, but did want to run this idea by you in a separate PR, will follow up
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.
This is so fast!!! But for me the display doesn’t change on rebuild, so I wasn’t sure if it was working or not (it was; just didn’t log). Maybe adding an update like Rebuilt App.tsx [0.01s]
?
Webpack's |
It could probably work, but it’d be a bit tricky. There’s basically HMR code that’s added to your JS in order to get this to work, so we’d have to make that even possible to write to disk (currently that only exists in memory in the dev server). But even if we got that to work, the particular HMR code we send may not work with every dev server (there’s no standard yet, but it’s something that @FredKSchott and others are working on). All that to say, it’s possible, but very tricky without controlling both build & dev server. |
yup, HMR is directly tied to the dev server, and the people who want this want to own their own dev server. So unfortunately we can't support HMR by default, out of the box. But... there are definitely some interesting things that you could do. For example, our HMR engine on the client connects to an HMR server via WebSocket, could we continue to host the Websocket connection in It would be fun to explore a bit more (happy to help anyone who wants to!) but ultimately out of scope for this PR / near term release. |
Yeah, that's the sort I think I had in mind - I wouldn't expect it to work by default, I'd definitely expect HMR to be opt-in for |
Resolves #376
Changes
build --watch
mode, our most requested feature! (snowpack build --watch mode #376)Testing