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 path resolution in Pages esbuild #773

Closed
wants to merge 1 commit into from

Conversation

JacobMGEvans
Copy link
Contributor

fix: the pathing for the template was resolving using __dirname into the esbuild file which then used a relative path back from pages. When ran inside a test would get a resolved absolute path to the template from "pages/pages/functions/template-workers.ts

…o the esbuild file which then used a relative path back from pages. When ran inside a test would get a resolved absolute path to the template from "pages/pages/functions/template-workers.ts
@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2022

🦋 Changeset detected

Latest commit: 743d6b2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2117932154/npm-package-wrangler-773

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/773/npm-package-wrangler-773

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2117932154/npm-package-wrangler-773 dev path/to/script.js

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Let's wait for @GregBrimble to confirm this is a bug before we merge.

@petebacondarwin
Copy link
Contributor

Oh interesting...

https://github.com/cloudflare/wrangler2/runs/5892721660?check_suite_focus=true#step:6:942

This implies that the original pathing is correct, at least for how that e2e test is setup.

entryPoints: [
path.resolve(__dirname, "../pages/functions/template-worker.ts"),
],
entryPoints: [path.resolve(__dirname, "./template-worker.ts")],
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if __dirname returns a different result once this file is bundled?

In other words, the __dirname at runtime is packages/wrangler/wrangler-dist rather than the source path of packages/wrangler/pages/functions.

I note that this only came up when we were trying to run Pages in a Wrangler unit test, which is not bundled...

@threepointone
Copy link
Contributor

Tests are failing too, expected?

@GregBrimble
Copy link
Member

Is this linked to an issue? The existing template resolution is working fine for me in prod and for local dev. This new one doesn't.

@petebacondarwin
Copy link
Contributor

So the problem is that esbuild does not rewrite __dirname when bundling. So it refers to the directory containing the bundle, rather than the directory containing the original source file.
This is problematic since you do not necessarily know where the final bundle will be run compared to the file that is being loaded.
Apparently there there is an esbuild plugin to solve this.

@JacobMGEvans
Copy link
Contributor Author

So the problem is that esbuild does not rewrite __dirname when bundling. So it refers to the directory containing the bundle, rather than the directory containing the original source file. This is problematic since you do not necessarily know where the final bundle will be run compared to the file that is being loaded. Apparently there there is an esbuild plugin to solve this.

I will take a look into implementing the plugin in this PR then we can discuss adding it to ESBuild for the Wrangler side as well.

@petebacondarwin
Copy link
Contributor

evanw/esbuild#859

@petebacondarwin
Copy link
Contributor

Any update on this @JacobMGEvans ?

@petebacondarwin petebacondarwin marked this pull request as draft April 19, 2022 09:20
@JacobMGEvans
Copy link
Contributor Author

Any update on this @JacobMGEvans ?

Not at the moment.

@JacobMGEvans
Copy link
Contributor Author

Closing due to work in #860

@JacobMGEvans JacobMGEvans deleted the pages-esbuild-template-pathing branch May 2, 2022 23:23
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.

4 participants