build with relative paths #173
Conversation
| assert.strictEqual(relativeUrl("/foo/", "/foo/"), "./"); | ||
| assert.strictEqual(relativeUrl("/foo", "/foo/"), "./foo/"); |
There was a problem hiding this comment.
could you explain the difference between source being /foo/ vs /foo?
/foo/ means you're in a directory under /foo vs in /foo
There was a problem hiding this comment.
I don't know if we should make the differentiation between "/foo" vs "/foo/". What is the use case for /foo/ without the nested directory (like /foo/bar). Would the target ever have a trailing slash?
There was a problem hiding this comment.
You get /foo/ if you have a foo/index.md. You get /foo if you have a foo.md.
Co-authored-by: Mike Bostock <[email protected]>
| const from = `/${source}`.split(/[/]+/g).slice(0, -1); | ||
| const to = `/${target}`.split(/[/]+/g); |
There was a problem hiding this comment.
It’s worth mentioning that relativeUrl can be called with two different types of paths with different characteristics: serving paths and source paths.
Serving paths, such as during render, start with a slash and don’t end with an extension. For example /javascript/displays.
Source paths, such as during rewriteImports, don’t start with a slash and do end with an extension. For example, javascript/displays.md. Source paths are typically relative to the source root, or to a subdirectory in source root (such as when computing transitive imports).
It looks like the code already handles both of these correctly, but I think it’s worth calling it out in the tests, and having explicit tests for each type of path. I think we also want more tests for the source path case.
Co-authored-by: Mike Bostock <[email protected]>
src/javascript/imports.ts
Outdated
| @@ -105,7 +106,7 @@ export function rewriteImports(output: any, rootNode: JavaScriptNode, sourcePath | |||
| : node.specifiers.find(isNamespaceSpecifier)?.local.name ?? "{}" | |||
| } = await import(${JSON.stringify( | |||
| isLocalImport(value, sourcePath) | |||
| ? join("/_import/", join(dirname(sourcePath), value)) | |||
| ? relativeUrl(sourcePath, join("/_import/", dirname(sourcePath), value)) | |||
There was a problem hiding this comment.
Just to avoid writing this twice, how about:
| ? relativeUrl(sourcePath, join("/_import/", dirname(sourcePath), value)) | |
| ? relativeImport(sourcePath, value) |
where
function relativeImport(sourcePath, value) {
return relativeUrl(sourcePath, join("/_import/", dirname(sourcePath), value));
}In theory you could then move the leading ./ requirement into relativeImport instead of relativeUrl, but I think it’s fine to leave it in relativeUrl for now too.
closes #42
Note: the "to" implementation is pretty lame, I'm sure there's a better way to compute the relative path from x to y.I ended up creating one without relying on node's path.relative (which is meant for files not URLs).