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

feat(custom-esbuild): add support for plugin configuration #1683

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

Conversation

spike-rabbit
Copy link

@spike-rabbit spike-rabbit commented Feb 20, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

esbuild plugin options cannot be provided

Issue Number: #1661

What is the new behavior?

esbuild plugin options can be provided using

{
   "plugins": ["without-options",  { "path" : "with-options", "options" : {}}]
}

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@spike-rabbit spike-rabbit force-pushed the feat/support-esbuild-plugin-config branch from 81890a5 to aa53635 Compare February 20, 2024 15:30
@spike-rabbit
Copy link
Author

Can you give me a hint, how I can actually run the examples without copying things around?

@just-jeb
Copy link
Owner

@spike-rabbit Once you run yarn everything should be linked, so if you built, for example, custom-webpack, you should be able to go to the folder with examples and run their build/test targets.
If you just want to run the whole set of tests then you can run yarn ci in custom-webpack package's root folder.

Did I answer your question or you meant something else?

@spike-rabbit spike-rabbit force-pushed the feat/support-esbuild-plugin-config branch 2 times, most recently from 8516e5d to 239abb4 Compare February 22, 2024 08:20
@spike-rabbit
Copy link
Author

Thx, I solved my problem. I should have run build before running the examples. Now it works

@spike-rabbit spike-rabbit force-pushed the feat/support-esbuild-plugin-config branch from 239abb4 to 6359159 Compare February 22, 2024 08:24
name: 'define-title',
setup(build) {
const options = build.initialOptions;
options.define.titleByOption = JSON.stringify(pluginOptions.title);
Copy link
Owner

Choose a reason for hiding this comment

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

Same here (remove JSON.stringify)

@@ -112,7 +112,7 @@ Builder options:

In the above example, we specify the list of `plugins` that should implement the ESBuild plugin schema. These plugins are custom user plugins and are added to the original ESBuild Angular configuration. Additionally, the `indexHtmlTransformer` property is used to specify the path to the file that exports the function used to modify the `index.html`.

The plugin file can export either a single plugin or a list of plugins:
The plugin file can export either a single plugin, a single plugin with configuration or a list of plugins:
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rephrase to:

The plugin file can export either a single plugin or a list of plugins. If a plugin accepts configuration then the config should be provided in `angular.json`:

@spike-rabbit spike-rabbit force-pushed the feat/support-esbuild-plugin-config branch from 6359159 to cefcaf9 Compare March 4, 2024 08:20
@spike-rabbit
Copy link
Author

@just-jeb I changed everything as requested.

Can you release the changes once this is merged?

@@ -100,7 +100,7 @@ Builder options:
"build": {
"builder": "@angular-builders/custom-esbuild:application",
"options": {
"plugins": ["./esbuild/plugins.ts", "./esbuild/plugin-2.js"],
"plugins": ["./esbuild/plugins.ts", { "path": "./esbuild/plugin-2.js", "options": { "key": "value" } }],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we please have a more practical example demonstrating its usefulness? For instance, could you provide an example involving a real plugin that necessitates options, or perhaps share a scenario from your own application illustrating why it's essential?

Copy link
Author

Choose a reason for hiding this comment

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

makes sense for me, but I found a bug which makes using 3rd party plugins (almost) impossible.

Paths are always joined with the workspaceRoot loadModule<Plugin | Plugin[]>(path.join(workspaceRoot, pluginPath), tsConfig, logger). This makes it impossible to load modules by the package name.

I would propose to remove this and just use the plugin path without the workspace root. Unless I missed something, this would be inline with the behavior of custom-webpack.
If you agree, I can do this in another MR.

Copy link
Owner

Choose a reason for hiding this comment

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

How about we merge it and have a separate PR for 3rd party plugins (and then update the example)?

Copy link
Owner

Choose a reason for hiding this comment

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

@spike-rabbit Hmmm I wonder if that's the reason for JSON.stringify (CI fails now):
image

Copy link
Author

Choose a reason for hiding this comment

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

  • JSON stringify is back
  • updated the README

@spike-rabbit spike-rabbit force-pushed the feat/support-esbuild-plugin-config branch from cefcaf9 to 6819f24 Compare March 7, 2024 07:32
// esbuild/plugins.ts
import type { Plugin, PluginBuild } from 'esbuild';

function defineRewritePathPlugin(options: { text: string }): Plugin {
Copy link
Collaborator

@arturovt arturovt Mar 7, 2024

Choose a reason for hiding this comment

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

This should be the esbuild/define-env.ts example, along with a practical illustration of how the stage parameter could be utilized. Please avoid providing text examples, as they are unnecessary.

The define.buildText only shows how to define the global compile-time constant, but the current showcase should illustrate how options are useful.

Copy link
Author

Choose a reason for hiding this comment

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

is updated

@spike-rabbit spike-rabbit force-pushed the feat/support-esbuild-plugin-config branch from 6819f24 to 81f145d Compare March 13, 2024 07:59
@just-jeb
Copy link
Owner

@arturovt is it all good from your perspective? Can we merge?

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.

None yet

3 participants