Skip to content

fix: work on environments without import.meta.resolve #13

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

Closed
wants to merge 1 commit into from

Conversation

avivkeller
Copy link

See nodejs/nodejs.org#7544 for reference.

Copy link
Owner

@remcohaszing remcohaszing left a comment

Choose a reason for hiding this comment

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

Thanks for chipping in, but this solution won’t work.

const resolve: (path: string) => string =
import.meta.resolve || ((path) => new URL(path, import.meta.url).href);

const html = resolve('../index.html')
Copy link
Owner

Choose a reason for hiding this comment

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

This is fine, but it could also be written as:

Suggested change
const html = resolve('../index.html')
const html = String(new URL('../index.html'))

const mermaidScript = {
url: import.meta.resolve('mermaid/dist/mermaid.js')
url: resolve('mermaid/dist/mermaid.js')
Copy link
Owner

Choose a reason for hiding this comment

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

This won’t work. This is a bare import specifier that must resolve into node_modules.

}
const faStyle = {
// We use url, not path. If we use path, the fonts can’t be resolved.
url: import.meta.resolve('@fortawesome/fontawesome-free/css/all.css')
url: resolve('@fortawesome/fontawesome-free/css/all.css')
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above.

@avivkeller
Copy link
Author

Ahhh I see your concerns, it seems I didn't think of that. Thanks for your reply! I'll see if there's another way to achieve this.

@remcohaszing
Copy link
Owner

I’m open to suggestions, but unfortunately I don’t see a path forward. I’m closing this PR for now.

The goal of this PR isn’t to make it work in environments that don’t support import.meta.resolve. The goal is to make the Node.js implementation work with a bundler. See nodejs/nodejs.org#7544 (comment)

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.

2 participants