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

docs(custom-esbuild): align the HTML transformer signature #1705

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arturovt
Copy link
Collaborator

@arturovt arturovt commented Mar 4, 2024

Closes #1690

@arturovt arturovt requested a review from just-jeb March 4, 2024 21:51
@just-jeb
Copy link
Owner

Not sure if it actually closes #1690 - it does align the docs with actual implementation but from what I read there is a request for adding options (similarly to custom-webpack builder).

@arturovt
Copy link
Collaborator Author

We can align the docs for now and then I’ll think on implementing options as a feature.

@just-jeb
Copy link
Owner

Sure thing, just saying that aligning the docs won't close #1690 🙂

@arturovt
Copy link
Collaborator Author

arturovt commented Mar 10, 2024

However, how do we include target options as the first parameter when the HTML transformer is called by Angular with only the HTML content? We are not responsible for its invocation with only a single parameter.

// angular-cli
return indexHtmlTransformer && content ? await indexHtmlTransformer(content) : content;

We can certainly make adjustments on our end, but I'm hesitant to modify the contract exposed by their API. If they didn't include target options for this builder, there might be a specific reason for it.

The target options only include target and configuration properties. Here, target can be either build or server, while configuration can be development, production, or any other value.

In this builder, the target is typically always set to build, as the application builder inherently possesses conjunctive functionality.

Which means the only property may be used is the configuration.

I can talk to one of the Angular CLI team members who worked on the application builder to understand the exact reasons for its removal. I'm not sure if they gonna decide to bring it back.

@shisukei
Copy link

@arturovt I'm not sure if this is ideal, but from what I've seen, at the post-bundle time, if there is an index transformation function specified, then this function is called with the index content as a parameter.

Considering that the context is available in buildCustomEsbuildApplication (or in executeCustomDevServerBuilder), wouldn't it be acceptable to wrap this in a custom function? For instance, if we take the current code of this file, we would replace line 30 with something like this:
return { codePlugins, indexHtmlTransformer: (indexHtml) => indexHtmlTransformer(context, indexHtml) } as ApplicationBuilderExtensions;

Not sure, but it seems to be a similar approach with custom-webpack (source code).

@just-jeb
Copy link
Owner

@arturovt Exactly what @shisukei said, we can create a wrapper function and use the target params from the closure, just like we do in custom-webpack.

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.

TargetOptions parameter is not there with custom-esbuild
3 participants