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 incorrect output path due to esbuild outbase. #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cameronhunter
Copy link

Context

esbuild calculates the lowest common ancestor of the entryPoints to determine where to place the output file. From their docs:

If your build contains multiple entry points in separate directories, the directory structure will be replicated into the output directory starting from the lowest common ancestor directory among all input entry point paths. For example, if there are two entry points src/home/index.ts and src/about/index.ts, the output directory will contain home/index.js and about/index.js. If you want to customize this behavior, you should change the outbase directory.

Issue

In watch mode, this plugin builds each added/changed file as a single entryPoint leading to the output files all being at the top-level of outdir. To fix the issue we need to calculate the lowest common ancestor of the resolved glob ourselves and make use of esbuild's outbase option to maintain the correct directory structure.

@@ -1,6 +1,10 @@
// eslint-disable-next-line @typescript-eslint/triple-slash-reference
/// <reference path = "./lowest-common-ancestor.d.ts" />
Copy link
Author

Choose a reason for hiding this comment

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

I've created a pull-request adding types for lowest-common-ancestor to @DefinitelyTyped, but they're awaiting approval. These lines can be deleted once it's merged.

@waspeer
Copy link
Owner

waspeer commented Aug 27, 2021

Thanks for the pr! :) I will take a look at it tomorrow

@waspeer
Copy link
Owner

waspeer commented Aug 28, 2021

Hi Cameron,

I've looked at the PR and worked on a test for it, but I ended up with a few reservations. I'm curious to hear your thoughts.

I'm wondering what the expected behavior is in watch mode. The way the fix works atm is it checks the state of the filesystem when the plugin first runs and then calculates the lowest common ancestor from it. What should be happening when a file is added or removed while the plugin is watching and it changes the lowest common ancestor? It could maintain the same initial outbase, but then the build would be different when you stop and start the build again (this is the current behavior). It could delete all the files in the outdir and rebuild with a new outbase, but I'm not sure that's desirable behavior.

I feel like a user would get the most predictable behavior by explicitly setting outbase themselves in watch mode. Maybe the documentation could include a warning that you'd probably want to set outbase when running in watch mode? Or it could even log a warning when the option is not set in watch mode?

Another thought I had is that this would be a breaking change. So we either have to put this functionality behind a configuration option or bump a major version. But given what I described above, I'm leaning more towards the documentation/warning route.

I'm really happy with the improved cwd support and the filesOnly option on the glob package. Those we're definitely some cases I missed!

@cameronhunter
Copy link
Author

Thanks for checking it out @waspeer.

In my mind I'd want watch mode to behave the same way as a standard build, so if a file change resulted in a different outbase I think that clearing the outdir and rebuilding everything would make sense. I like your suggestion of prompting the user that the calculated outbase changed in that scenario too – it's informative rather than feeling like magic.

Explicitly setting an outbase is certainly the safest route but I feel the option is easily overlooked in the esbuild docs as it's categorized in the "Advanced options" section.

I agree that this would be a breaking change.

@waspeer
Copy link
Owner

waspeer commented Sep 2, 2021

Ok! I'll play around with it and try to come up with an intuitive behavior. Could you help me out with one thing, though? Could you sketch out a situation in which you needed this option? I'd like to model a test to such a situation and work on a solution from there.

This is what I've come up with:

// Input
foo /
  file1.ts
bar /
  file2.ts

// Expected output
foo /
  file1.js
bar /
  file2.js

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