alternative approach to #77 for ssr reload detection

[Rich-Harris]

This is an alternative approach to #77 — rather than comparing a module and all its dependencies to the cache to determine whether it's stale, we use a filewatcher to proactively evict files from the cache as necessary.

I'd argue it's a bit simpler (allows us to ditch the hash stuff altogether), and more closely resembles what Snowpack itself is doing. Thoughts @ehrencrona?

cc @FredKSchott — maybe there'd be some value in exposing an API for this?

export default function loader(snowpack: SnowpackDevServer): Loader {
  const cache = new Map<string, Promise<any>>();
  const graph = new Map<string, Set<string>>();

  const invalidate_all = path => {
    cache.delete(path);

    const dependents = graph.get(path);
    graph.delete(path);

    if (dependents) dependents.forEach(invalidate_all);
  };

  snowpack.onInvalidate(invalidate_all);

  // ...

(Though if there's any plan to eventually bake the loading logic into Snowpack itself, then there'd be no need to expose an invalidation API, so this might be premature.)

[ehrencrona]

I'm not sure it will simplify things. It might, but not necessarily. We would still need to track dependencies. The worries about race conditions is the really tricky part and they wouldn't go away. They might become worse by having another event (the watcher firing) happening asynchronously.

Once the watcher triggers, can we guarantee that Snowpack will immediately respond with the new version of the file, or is there a risk of still getting the old version for a short period?

Ideally, I'd like to see some kind of "if modified since" API for snowpack so that we could just ask directly "has this file changed?" rather than hashing it and comparing to the cache.

[Rich-Harris]

Dependencies are tracked, that's what this line does:

graph.get(pathname).add(importer);

I'm not sure I understand the concerns about race conditions? Can you describe a situation in which this might break?

Once the watcher triggers, can we guarantee that Snowpack will immediately respond with the new version of the file

Yes

Ideally, I'd like to see some kind of "if modified since" API for snowpack so that we could just ask directly "has this file changed?" rather than hashing it and comparing to the cache.

There is no more hashing in this PR!

[ehrencrona]

Sorry, I somehow thought this was an issue and not a PR, so I never saw the code and thought you were talking about an abstract idea. No, this is of course lower complexity than my suggestion.

I see two possible race conditions in this code:

• two requests for same file coming in close to each other in time can cause the file two be loaded twice (because there's an await before the cache is set). This is easy to fix by making the load part of the cached promise.
• the graph data structure can get screwed up if it is invalidated while the module is initializing. I don't think it should lead to bugs, since it would be overwritten on the next load.

Another thing that strikes me: this assumes that the module structure is the same as the file structure. Is that always the case? E.g. symbolic links, directory aliases and preprocessing

[FredKSchott]

Happy to expose an API for this, and I'd actually recommend that you use it. Not just to remove the need for two file watchers, but also because Snowpack plugins have the ability to respond to file changes, and can potentially mark other files as changed. See https://www.snowpack.dev/guides/plugins#onchange() and https://www.snowpack.dev/guides/plugins#this.markchanged()

Sass is our best illustration of this, where a change to a helper file like _base.scss needs to mark all importers of that file as no longer valid aka changed, so that those importers get rebuilt and the change gets picked up by HMR, etc. Not sure if that's directly relevant for a Svelte project, but other plugins can do similar things.

I'll probably go ahead and add this to Snowpack via PR regardless, but it is up to you all to use it or not!

[benmccann]

It looks like this is the new API that @FredKSchott suggested he might add that we can use here: FredKSchott/snowpack#1452

[ehrencrona]

Nice! I tried switching to the new API in #130 and it seems to work.

[benmccann]

I'l close this in favor of #130 since using the new API seems like a sensible way to go. Feel free to reopen this if you disagree with that Rich

[benmccann]

I'm not quite sure what happened with #130. It looks like it was merged into this branch, so I'm reopening this PR so that we can get the change into master

[benmccann]

@Rich-Harris can you drop pnpm-lock.yaml from this PR or rebase against master?