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

import.meta.resolve for files #1696

Merged
merged 6 commits into from
Sep 30, 2024
Merged

import.meta.resolve for files #1696

merged 6 commits into from
Sep 30, 2024

Conversation

mbostock
Copy link
Member

Fixes #1008. When import.meta.resolve is passed a relative path to a non-JavaScript file (anything other than .js, .mjs, or .cjs — and the latter two we don’t expect in practice), it now uses resolveFile instead of resolveImport to resolve.

@mbostock mbostock requested a review from Fil September 29, 2024 16:12
@mbostock mbostock enabled auto-merge (squash) September 29, 2024 17:16
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.

in preview, this does not seem to apply to "volcano.json" without a leading ./:

FileAttachment("volcano.json").href // "http://127.0.0.1:3000/_file/volcano.json?sha=f5223ec98032634f121adef600c947fab5ab9389d689a0bb1e21d6a877d21178"

import.meta.resolve("volcano.json") // "volcano.json" 🌶

import.meta.resolve("./volcano.json") // "http://127.0.0.1:3000/_file/volcano.json?sha=f5223ec98032634f121adef600c947fab5ab9389d689a0bb1e21d6a877d21178"

and in build, it crashes on import.meta.resolve("./volcano.json"):

copy docs/volcano.json →
Unexpected error: Unexpected token (1:8)
The full error follows

file:///Users/fil/Source/framework/node_modules/acorn/dist/acorn.mjs:3580
  var err = new SyntaxError(message);
            ^

Also, can we mention it in docs/files.md?

@mbostock
Copy link
Member Author

The error is expected. You need to using a leading dot-slash to import a relative path (./, /, or ../). Otherwise volcano.json is interpreted as a bare module specifier (like d3 or npm:d3). I can work on a better error message.

@mbostock

This comment was marked as resolved.

@Fil
Copy link
Contributor

Fil commented Sep 30, 2024

I think I'm seeing a different bug, as I get a crash (copy docs/volcano.json → \nUnexpected error: Unexpected token (1:8)) if I add

```js echo
import.meta.resolve("./volcano.json")
```

to docs/files.md, and run yarn docs:build.

However it doesn't seem completely related to this PR, or it's my testing that is wrong, because I see the same issue on main with this minimal repro: https://github.com/observablehq/broken-build/blob/main/src/index.md (I'm a bit puzzled.)

@mbostock
Copy link
Member Author

I’m able to reproduce the error, thanks. Investigating now.

@mbostock
Copy link
Member Author

Okay, fixed. 👍

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.

I guess it's missing documentation, but LGTM

@mbostock mbostock merged commit 83d21f2 into main Sep 30, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/meta-resolve-file branch September 30, 2024 19:14
@Fil
Copy link
Contributor

Fil commented Sep 30, 2024

oops auto-merge

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.

import.meta.resolve for files
2 participants