Skip to content
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

Add HMR support for linked & local npm packages #470

Merged
merged 1 commit into from Jun 11, 2020
Merged

Conversation

drwpow
Copy link
Collaborator

@drwpow drwpow commented Jun 10, 2020

From #429, Adds basic HMR support for linked & local npm packages. As seen here:

2020-06-10 16-10-41 2020-06-10 16_11_16

The basic process is this:

  1. Start the dev server with snowpack dev
  2. Snowpack looks through your installed dependencies, and it looks for dependencies that are linked or local (file:…). It knows because some metadata is missing when it’s not downloaded from npm.
  3. Snowpack then watches these directories—and only these directories—and when a file changes within, it does a fresh install and then reloads the dev server.

In the future, this could possibly be extended to support all first-class dependencies in node_modules, but for now it’s limited to linked & local packages (i.e. local to your filedisk, not downloaded).

@drwpow drwpow requested a review from a team as a code owner June 10, 2020 22:14
@@ -198,7 +198,7 @@ function resolveWebDependency(dep: string, isExplicit: boolean): DependencyLoc {
}
return {
type: 'JS',
loc: path.join(depManifestLoc, '..', foundEntrypoint),
loc: path.join(depManifestLoc || '', '..', foundEntrypoint),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were because of the return type addition.

@drwpow drwpow force-pushed the drwpow/npm-link-hmr branch 3 times, most recently from 5120b88 to bec4c02 Compare June 10, 2020 22:25
@drwpow drwpow requested a review from FredKSchott June 10, 2020 22:25
cwd: '/', // we’re using absolute paths, so watch from root
persistent: true,
ignoreInitial: true,
disableGlobbing: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up not needing the followSymlinks: true here! Because we’re not watching the symlink dirs 😛

@drwpow drwpow force-pushed the drwpow/npm-link-hmr branch 2 times, most recently from b959efc to c353f51 Compare June 10, 2020 22:35
@@ -214,6 +223,28 @@ export async function command(commandOptions: CommandOptions) {
// no import-map found, safe to ignore
}

/** Rerun `snowpack install` while dev server is running */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small note: I’ve recently gotten into more JSDoc-style comments because they leave these nice notes in VS Code:

Screen Shot 2020-06-10 at 4 37 42 PM

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I'd like to start doing the same

@drwpow drwpow force-pushed the drwpow/npm-link-hmr branch 4 times, most recently from f53d6bc to b21a131 Compare June 11, 2020 00:26
Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some small comments (since we paired on some of this, not too much new code to review).

@@ -214,6 +223,28 @@ export async function command(commandOptions: CommandOptions) {
// no import-map found, safe to ignore
}

/** Rerun `snowpack install` while dev server is running */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I'd like to start doing the same

src/commands/dev.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@airhorns
Copy link

airhorns commented Jun 11, 2020

Quick question about this change: will it have any impact on https://www.pika.dev/npm/snowpack/discuss/286 , or could it be extended to support that use case? Those packages are also local and liable to change, but they aren't sourced via file: urls in a package.json I don't think.

@drwpow
Copy link
Collaborator Author

drwpow commented Jun 11, 2020

@airhorns Thanks for raising that discussion! I think yeah that may be another PR to add Yarn workspaces. I haven’t used it much but as you pointed out I think it’s just different enough from yarn link where I’d want to write a test specifically for that setup.

Also whereas this PR only touches snowpack dev for linked packages, I think workspaces may require a change to dev and build (maybe).

@drwpow drwpow merged commit 3280de3 into master Jun 11, 2020
@drwpow drwpow deleted the drwpow/npm-link-hmr branch June 11, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants