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

refactor: use esbuild "file" loaders to add runtime files to the published assets #860

Closed

Conversation

petebacondarwin
Copy link
Contributor

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Apr 29, 2022

🦋 Changeset detected

Latest commit: e6c3c60

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 29, 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/2250030696/npm-package-wrangler-860

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

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

Or you can use npx with this latest build directly:

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

@petebacondarwin petebacondarwin force-pushed the runtime-assets branch 3 times, most recently from b37204e to 1519b10 Compare April 29, 2022 17:42
…ished package

When esbuild builds Wrangler, it creates a new directory "wrangler-dist", which
contains the bundled output, but then any `__dirname` or similar Node.js constant
used in the code, is relative to this output directory.

During testing, Jest does not bundle the code, and so these `__dirname` constants
are now relative to the directory of the original source file.

This is a refactor that ensures consistency in the access of runtime files
between production and test environments, by implementing build/test time transforms.
In esbuild we use a "file" loader that will not inline the content of the imported file
into the bundle, but instead copy the content to the output directory, and then return
the path to this file in the code. We can then use this value to load the content at
runtime.

Similarly, in Jest testing, we have a custom transform that will return the location of the
original file (since there is no real output directory to copy the file to).

The result of this change, is that we can just ensure that the paths in the source code are
relative to the source file and not worry about where the generated code will be output.
Further we are able to remove a bunch of files from the package that we publish to npm.
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

The reasoning is good, but I don't like the implementation of using require to instead inline the path to the file. It's confusing if you read it and you don't know what's going on. Instead, I recommend making a new virtual module (say, wrangler-template-paths) that exports an object to these paths.

import paths from 'wrangler-template-paths';
paths['new-worker.template.js']; // /asbolute/path/to/new-worker.template.js
// etc

Implementing this would be an esbuild plugin that captures imports to wrangler-template-paths and sends an object with those paths as its content. What do you think?

@petebacondarwin
Copy link
Contributor Author

Is your concern the use of require() rather than import? Or the fact that these are dotted around the codebase?

@threepointone
Copy link
Contributor

threepointone commented May 2, 2022

The concern us around using require, but I'd feel this way about using import as well. It's extremely curious to see a require/import of a ts/json file result in a string path to that file. file loaders are typically used for static assets, and even that is a bit icky, but is a better option until we have import assertion attribute evaluators (or any other standard that passes). But using it for .ts/.json files feels a bit wrong. More concernedly, it'll be confusing for anyone who comes by it in the future and doesn't understand what it's doing.

Here's another alternative -

pathToTemplateFile('path/to/new-worker.ts'); // where `pathToTemplate` is a magic global function that works in our codebase

^ If this would desugar to a require() that uses a file loader to do the same thing you mentioned, (using define in the esbuild config) I would feel much better about it, since it's a little more obvious what's going on.

Copy link
Contributor

@JacobMGEvans JacobMGEvans left a comment

Choose a reason for hiding this comment

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

This is a great improvement not just on pathing but how we bundle files we need i.e. templates

If it is not already done I will close the PR that I started to initially handle the pathing for pages template, this PR handles that and more!

Edit: Provisional to Greg's approval as well as resolution in conversation pertaining to functional abstraction pathToTemplate

@petebacondarwin
Copy link
Contributor Author

This PR is very stale and not likely to land soon as it has limited impact and requires more discussion.

@petebacondarwin petebacondarwin deleted the runtime-assets branch June 28, 2023 07:31
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.

3 participants