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

fix: Fails to load import map with module URL starting with "./" #162

Merged
merged 2 commits into from
Jan 12, 2022

Conversation

aiotter
Copy link
Contributor

@aiotter aiotter commented Jan 10, 2022

Lume fails to parse import map like following:

{
  "imports": {
    "/": "./"
}

// Import map diagnostics:
//   - Invalid address "./" for the specifier key "/".

This is because Lume parses the given import map and convert it to data URI scheme.
Data URI scheme has no base URL, so parsing this specifier fails.

lume/ci.ts

Lines 45 to 70 in 5f0d950

export async function getImportMap(mapFile?: string) {
const map: ImportMap = {
imports: {
"lume": join(baseUrl, "/mod.ts"),
"lume/": join(baseUrl, "/"),
"https://deno.land/x/lume/": join(baseUrl, "/"),
},
};
if (mapFile) {
try {
const url = await ensureUrl(mapFile);
const file = await (await fetch(url)).text();
const parsedMap = JSON.parse(file) as ImportMap;
map.imports = { ...map.imports, ...parsedMap.imports };
map.scopes = parsedMap.scopes;
} catch (cause) {
throw new Exception("Unable to load the import map file", {
cause,
mapFile,
});
}
}
return `data:application/json;base64,${encode(JSON.stringify(map))}`;
}

On deno environment, module URL which starts with "./" should be resolved to the import map's URL.
https://deno.land/manual/linking_to_external_code/import_maps

This PR fixes this issue by resolving those relative paths into file URI schemes.

Module URL starting with "./" should be parsed to a relative URL from
the json file of the import map.
ci.ts Outdated Show resolved Hide resolved
@aiotter
Copy link
Contributor Author

aiotter commented Jan 12, 2022

I've never seen checkout@v2 fails.
Can you re-run the workflow? I believe this is a temporary problem.

@oscarotero oscarotero merged commit c7bde37 into lumeland:master Jan 12, 2022
@oscarotero
Copy link
Member

Thanks, it looks good.
I think the test error is not related with this.

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