Skip to content

hot module replacement for local imports! #235

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

Merged
merged 21 commits into from
Nov 26, 2023
Merged

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Nov 23, 2023

This computes the content hash of an imported ES module and its transitive imports. This ensures that when you import a.js, and a.js imports b.js, and b.js changes, that you get a new copy of a.js.

In addition, in refreshAttachment we now detect when an import references a changed file; in this case, instead of simplying send a refresh↓, we trigger a recompilation of the JavaScript to resolve new content hashes and rediscover which files to watch. (I took the lazy route here and re-parsed the entire Markdown file, but that should be fast enough since we do it on every save anyway.)

Fixes #146.

@mbostock mbostock force-pushed the mbostock/import-sha branch 2 times, most recently from 3d66f51 to 985eeb3 Compare November 23, 2023 21:46
@mbostock mbostock force-pushed the mbostock/import-sha branch from 985eeb3 to 0d15763 Compare November 24, 2023 14:37
@mbostock mbostock changed the title apply content hash to local imports hot module replacement for local imports! Nov 24, 2023
@mbostock mbostock marked this pull request as ready for review November 24, 2023 15:13
@mbostock mbostock requested review from Fil and cinxmo November 24, 2023 15:13
@@ -1,6 +1,6 @@
import "https://cdn.jsdelivr.net/npm/d3/+esm";
import {bar} from "../bar/bar.js";
export {top} from "../_import/top.js";
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing a bug!

@mbostock

This comment was marked as resolved.

src/files.ts Outdated
@@ -18,20 +18,10 @@ export function fileReference(name: string, sourcePath: string): FileReference {
return {
name,
mimeType: mime.getType(name),
path: normalizeRelativePath(relativeUrl(sourcePath, `/_file/${dirname(sourcePath)}/${name}`))
path: relativeUrl(sourcePath, `/_file/${dirname(sourcePath)}/${name}`)
Copy link
Member Author

@mbostock mbostock Nov 24, 2023

Choose a reason for hiding this comment

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

Should this check whether the name starts with a slash, like we do elsewhere? And we should probably use join to normalize the path (though it doesn’t hurt to normalize again within relativeUrl).

Related #42 #193.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was a bug. Fixed!

@mbostock mbostock enabled auto-merge (squash) November 26, 2023 03:37
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

A side effect is that the file's hash is baked in the build:

const {foo} = await import("../_import/javascript/foo.js?sha=92cd58cd10d9a20a34eaa4ac95ca324f16e33da431bd103f6afb019e2933be7e");

I don't think it's a problem (it even ensures cache busting on poorly configured hosts). But if it's intended as a feature, maybe mention it in the documentation?

@mbostock mbostock merged commit 59b4d0b into main Nov 26, 2023
@mbostock mbostock deleted the mbostock/import-sha branch November 26, 2023 11:36
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.

Editing an imported ES module should trigger a reactive update
2 participants