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

feat: save import map URL to manifest #235

Merged
merged 7 commits into from
Dec 6, 2022
Merged

feat: save import map URL to manifest #235

merged 7 commits into from
Dec 6, 2022

Conversation

eduardoboucas
Copy link
Member

Which problem is this pull request solving?

Import maps aren't working as expected at the moment for a couple of reasons.

First of all, we're not setting the import_map_url property in the isolate config, because we're not even writing that to the manifest.

Also, we're writing absolute paths (from the host machine in the build system) into the import map, instead of converting those to ESZIP paths using the virtual root prefix.

This PR addresses that.

List other issues or pull requests related to this problem

Closes https://github.com/netlify/pod-compute/issues/313.

@eduardoboucas eduardoboucas added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Dec 5, 2022
@eduardoboucas eduardoboucas requested a review from a team December 5, 2022 23:28
@@ -10,4 +14,20 @@ const url = new URL(import.meta.url)
const dirname = fileURLToPath(url)
const fixturesDir = resolve(dirname, '..', 'fixtures')

export { fixturesDir, testLogger }
const useFixture = async (fixtureName: string) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I created this helper and refactored all tests in bundler.test.ts to use it because I realised our tests were not mimicking the structure observed in production. In production, the base path (e.g. /project) is a parent to both the source directory (/project/netlify/edge-functions) and the dist directory (/project/.netlify/edge-functions-dist), but in tests we were using as dist directory a temporary path that is something like /var/tmp.

With this helper, we copy the source directory into that temporary directory and make the base path.

}

add(file: ImportMapFile) {
this.files.push(file)
}

getContents() {
let imports: Record<string, URL | null> = {}
getContents(rootPath?: string) {
Copy link
Contributor

@danez danez Dec 6, 2022

Choose a reason for hiding this comment

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

Shouldn't rootPath be mandatory? For example .toDataURL() would now create a different import map?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't have relative paths when the import map is described as a data URL, so in that situation we'll leave the paths as absolute.

@kodiakhq kodiakhq bot merged commit 94c6ec6 into main Dec 6, 2022
@kodiakhq kodiakhq bot deleted the feat/import-map branch December 6, 2022 13:45
Skn0tt pushed a commit to netlify/build that referenced this pull request Apr 23, 2024
* feat: save import map URL to manifest

* fix: use POSIX paths

* chore: fix integration test

* chore: fix test

* chore: remove unused file

* chore: fix path in test

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants