Various Esbuild improvements #25030
Merged
Various Esbuild improvements #25030
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dgp1130
added
action: review
|
The formatter seems to be conflicting with the linter here? Linter complains that an error message exceeds the line length, but if I break it into two lines, the formatter collapses them back on to one. Not sure how to handle that? |
You'd need to split the message into multiple strings. |
alan-agius4
reviewed
Apr 18, 2023
packages/angular_devkit/build_angular/src/builders/browser-esbuild/schema.json
Outdated
Show resolved
Hide resolved
dgp1130
force-pushed
the
esbuilder-improvements
branch
3 times, most recently
from
7c241bf
to
8692660
Compare
clydin
approved these changes
Apr 19, 2023
packages/angular_devkit/build_angular/src/builders/browser-esbuild/schema.json
Outdated
Show resolved
Hide resolved
dgp1130
force-pushed
the
esbuilder-improvements
branch
from
8692660
to
2251418
Compare
As is, the `browser` and `browser-esbuild` schemas needed to be identical to check for unsupported properties. This loosens it slightly by casting the `browser-esbuild` schema into the `browser` schema and looking for invalid properties. This makes the module more flexible as the `browser-esbuild` schema evolves _and_ more closely aligns with the actual mistake of users who incorrectly think of `browser-esbuild` as supporting all the options of `browser`, which it does not yet do so.
dgp1130
force-pushed
the
esbuilder-improvements
branch
from
2251418
to
f9b58ff
Compare
|
I also added another commit to support absolute file paths in entry points. Hopefully nothing too objectionable there, can you take another quick look @clydin? |
dgp1130
force-pushed
the
esbuilder-improvements
branch
from
f9b58ff
to
19d76aa
Compare
alan-agius4
requested changes
Apr 20, 2023
packages/angular_devkit/build_angular/src/builders/browser-esbuild/schema.json
Outdated
Show resolved
Hide resolved
packages/angular_devkit/build_angular/src/builders/browser-esbuild/options.ts
Show resolved
Hide resolved
packages/angular_devkit/build_angular/src/builders/browser-esbuild/schema.json
Outdated
Show resolved
Hide resolved
alan-agius4
reviewed
Apr 21, 2023
packages/angular_devkit/build_angular/src/builders/browser-esbuild/options.ts
Show resolved
Hide resolved
alan-agius4
reviewed
Apr 21, 2023
dgp1130
force-pushed
the
esbuilder-improvements
branch
from
19d76aa
to
399f489
Compare
alan-agius4
approved these changes
Apr 24, 2023
packages/angular_devkit/build_angular/src/builders/browser-esbuild/options.ts
Outdated
Show resolved
Hide resolved
...angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/entry-points_spec.ts
Show resolved
Hide resolved
...angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/entry-points_spec.ts
Outdated
Show resolved
Hide resolved
...angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/entry-points_spec.ts
Outdated
Show resolved
Hide resolved
...angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/entry-points_spec.ts
Outdated
Show resolved
Hide resolved
clydin
approved these changes
Apr 24, 2023
packages/angular_devkit/build_angular/src/builders/browser-esbuild/index.ts
Outdated
Show resolved
Hide resolved
This makes the `main` parameter optional and allows multiple entry points instead. `main` is still technically required by the schema, since it should almost always be set when invoked by a user. However, it now supports `null` as a value so it can be explicitly omitted. Longer term, we may choose to remove `main` and fold it into `entryPoints`, but for now we want to keep compatibility with the existing `browser` builder. Since `entryPoints` is an internal-only options (cannot be set in `angular.json` and isn't exposed in the schema), I made a new `buildEsbuildBrowserInternal()` function which adds the extra private option. This way direct invocations of the builder can provide this extra information without compromising the public API surface defined in the schema.
This allows `browser-esbuild` to consume absolute file paths and `entryPoints`. Absolute paths will always output in the root of the output directory with the same basename, since they are not within the workspace root and cannot exist at any guaranteed unique relative path. No attempt is made to check if the absolute path is actually within the workspace root, since this would require a call to `fs.realpath()` and make this logic dependent on the actual file system structure which introduces a lot of complexity we'd rather avoid. Longer term, the ideal approach is probably to leverage virtual files in some capacity, but this should be sufficient for now. `main` functionality is left alone, and absolute paths like `/main.ts` are treated as relative to the workspace root. This is to preserve existing functionality and discourage public API usage of file paths outside the workspace.
For testing use cases, we don't need an `index.html` file in the same capacity as a typical application. The builder already omits an `index.html` page when not set, this just updates the schema to reflect that. The parameter could be left optional, rather than required but allowing `false`. However doing it this way prevents users from accidentally forgetting to provide an index while still allowing users to explicitly disable index generation. We use `false` instead of `null` so users can write `--no-index` on the command line and get the same behavior, which would not be possible with `null`.
The `outExtension` option allows users to generate `*.mjs` files, which is useful for forcing ESM execution in Node under certain use cases. The option is limited to `*.js` and `*.mjs` files to constrain it to expected values. `*.cjs` could theoretically be useful in some specific situations, but `browser-esbuild` does not support that output format anyways, so it is not included in the type. I also updated `index.html` generation, which will correctly insert a `<script />` tag with the `*.mjs` extension. I opted to explicitly ban a "non-module" `*.mjs` file, since that would be very counterintuitive and I can't think of a valid use case for it.
dgp1130
force-pushed
the
esbuilder-improvements
branch
from
399f489
to
c1dc755
Compare
dgp1130
removed
the
action: review
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
browser-esbuildoptions are now able to differ slightly from the existingbrowserbuilder. This is to support adding additional functionality not currently supported by thebrowserbuilder, staying compatible with the existing builder is still a primary goal of the project.browser-esbuildnow supports an additionalentryPointsoption in itsInfrastructureSettings.mainandentryPointsnow support absolute file paths. Absolute files paths are always output with the same basename in the root of the output directory.browser-esbuildnow allowsindexto be optional. It already supportedindexnot being set, this just updates the schema.browser-esbuildnow supports anoutExtensionoption to configure the output file extension.Does this PR introduce a breaking change?