feat: support async plugin's pre/post by timofei-iatsenko · Pull Request #16862 · babel/babel

timofei-iatsenko · 2024-09-26T09:44:42Z

Q	A
Fixed Issues?	Fixes #16860
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

Enable support of async pre/post functions in plugins + update tests. As discussed here #16860

I'm also wondering of adding a isAsync flag for pre/post functions arguments so plugins could adjust it's own logic based on that. However i don't know how exactly to shape this signature, my proposition is:

  pre?: (this: S, file: File, ctx: {isAsync: boolean}) => void | Promise<void>;
  post?: (this: S, file: File, ctx: {isAsync: boolean}) => void | Promise<void>;

Let me know what you think, maybe there is a better place for that.

timofei-iatsenko · 2024-09-26T09:46:25Z

packages/babel-core/test/fixtures/async/plugin-post/plugin.js

      await wait(50);
+      this.file.ast.program.body[0].value = "success"


checking that promise is actually awaited

babel-bot · 2024-09-26T09:46:48Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58233

packages/babel-core/src/transformation/index.ts

nicolo-ribaudo

This looks good -- can you also ad a test with an async pre, which transforms the AST in such a way that makes sure that it's being awaited before running the visitors?

timofei-iatsenko · 2024-09-30T08:41:48Z

@nicolo-ribaudo there is one, it reassigns variable in the closure. If pre would not be awaited, visitor will print "failure" instead of "success".

I also have question regarding typings. Should we update typings of pre/post to reflect that they are supporting Async result?
I tried to change them, but this caused a lot of TS errors, and i honestly don't know how to solve them properly in this codebase without big changes.

nicolo-ribaudo · 2024-09-30T22:30:22Z

I also have question regarding typings. Should we update typings of pre/post to reflect that they are supporting Async result?
I tried to change them, but this caused a lot of TS errors, and i honestly don't know how to solve them properly in this codebase without big changes.

I tried changing them in packages/babel-core/src/config/validation/plugins.ts, and the type failure actually uncovered a bug!

Where we do chain(inherits.pre, plugin.pre), we are calling the two pre functions one after the other without awaiting if the first one is async. I think something like this should work (but I didn't test it):

function chainMaybeAsync<Args extends any[]>(
  a: undefined | ((...args: Args) => void | Promise<void>),
  b: undefined | ((...args: Args) => void | Promise<void>),
) {
  if (!a) return b;
  if (!b) return a;

  return function (this: unknown, ...args: Args) {
    const res = a.apply(this, args);
    if (res != null && typeof (res as Promise<void>).then === "function") {
      return (res as Promise<void>).then(() => b.apply(this, args));
    }
    return b.apply(this, args);
  };
}

You can add a test with a plugin A that has plugin B in its inherits, plugin B's pre is async, and plugin A's pre must run after it.

timofei-iatsenko · 2024-10-02T15:44:52Z

@nicolo-ribaudo i changed types for pre/post and fixed chaining. Also added two more tests for pre/post with inheritance.

The pipelines are failing for unknown reason. Could you check what's wrong?

nicolo-ribaudo

It just needed a rebase.

I left a minor comment, but this looks good now!

nicolo-ribaudo · 2024-10-02T18:32:03Z

packages/babel-core/test/fixtures/async/plugin-post-chaining/plugin.js

+  return {
+    inherits: pluginC,
+    async post() {
+      await wait(50);


Could you make pluginC async and keep pluginB sync? So we have async-sync-async, testing both combinations :)

Done, test for pre uses sync-async-async and test for post uses async-sync-async.

timofei-iatsenko · 2024-10-03T10:32:33Z

@nicolo-ribaudo what about passing isAsync to the plugin's authors? It could be passed directly to the pre/post function as a parameter:

pre?: (this: S, file: File, ctx: {isAsync: boolean}) => void | Promise<void>;
post?: (this: S, file: File, ctx: {isAsync: boolean}) => void | Promise<void>;

Or might be as a property on a PluginPass object?

I'm afraid it would be a bit tricky to implement no-breaking logic for the plugins without that flag.

nicolo-ribaudo · 2024-10-09T14:03:07Z

Whops sorry, I missed the question. I would prefer to expose it on PluginPass.

timofei-iatsenko · 2024-10-20T10:47:12Z

@nicolo-ribaudo i've added a new property isAsync on a BabelPass object. Corresponding tests for the functionality added.

timofei-iatsenko · 2024-10-20T10:48:33Z

packages/babel-core/src/transformation/plugin-pass.ts

+  /**
+   * The working directory that Babel's programmatic options are loaded
+   * relative to.
+   */
  cwd: string;

-  // The absolute path of the file being compiled.
+  /** The absolute path of the file being compiled. */
  filename: string | void;


With /** style of comment it's a jsdoc description which could be used as quick documentation in IDE.

timofei-iatsenko · 2024-10-20T10:51:48Z

packages/babel-core/src/transformation/plugin-pass.ts

+    file: File,
+    key?: string | null,
+    options?: Options,
+    isAsync?: boolean,


i've added isAsync as optional argument to the last position, so it shouldn't be a breaking change to anyone who uses PluginPass constructor directly. However, i would like to add it as required argument after the file. Let me know if that change is acceptable.

We only export PluginPass as a type and not as a constructor (see packages/babel-core/src/index.ts), so it should be fine to add it as a required property here.

Actually, you can just remove the ?s from the other arguments since they are always always passed. It's probably a leftover from when we converted the repo from Flow to TS.

timofei-iatsenko · 2024-10-20T10:52:32Z

packages/babel-core/test/async.js


  describe("misc", () => {
-    it("unknown preset in config file does not trigget unhandledRejection if caught", async () => {
+    it("unknown preset in config file does not trigger unhandledRejection if caught", async () => {


fixed a typo

JLHwung · 2024-10-24T15:47:37Z

packages/babel-core/src/config/full.ts

-      fn.apply(this, args);
+function chainMaybeAsync<Args extends any[], R extends void | Promise<void>>(
+  a: undefined | ((...args: Args) => R),
+  b: undefined | ((...args: Args) => R),


Here we assume a and b share the same return type, however, it is also possible that a is sync while b is async. I suggest we remove the R constraint and type them as (...args: Args) => void | Promise<void>. We can probably get rid of the final type cast as well.

For situations where both a and b are sync, we can add a type overload so that the chained function is still sync.

I think the current typing is fine:

if a and b are sync/undefined, R is void and thus chainMaybeAsync returns (...args: Args) => void.

if a is sync/undefined and b is async/undefined, R is void | Promise<void> and thus chainMaybeAsync returns (...args: Args) => void | Promise<void>. Note that the void | is correct, because if b is undefined then chainMaybeAsync returns a which returns void.

if a and b are async/undefined, R is Promise<void> and thus chainMaybeAsync returns (...args: Args) => Promise<void>.

Also note that this function is only called when a and b are either both sync, or both void | Promise<void>. We don't use it in mixed-type cases.

Also, a problem with a type overload is that every function is assignable to () => void, so it would also cover the case returning a promise:
https://www.typescriptlang.org/play/?#code/CYUwxgNghgTiAEAzArgOzAFwJYHtXzAAsotUBZKATwCMQBBAZ0vQB46YBzB+EADwxCpg3KKkoBtALoA+ABQAoePCgAueGlCJSIYPAA+8WbIB0p2FzXsuASngBeafABuOLMGsAaRfGpqNILVQdfUMTM04GSwjbB2dXdy9rNTDjc0j4KwYYxxc3AG55UEhYBBR0bDwCYlIKGnomVkyefkFhZTEpOW9VdSEA7V0DI1NUiKibexz4kIAFGBwAWywGEBZc4GlPb19ezQGQ4fCLDOjJuLdZ+aWVtfjNxOSRtPGss-XLxeXV9ekC+ULwNA4PAICAMEgAIzJbLnQbwOafG4-P6g8Fwbh2KokchUWiMZhgWSICEeSHWArwAD0lIAegB+IA

On the mixed-type cases, for example, if inherits.pre is a sync function and plugin.pre is an async function, then the merged pre will be an async function.

Like you said, every function is assignable to () => void, so the type checking can't help very much here. I am fine with leaving it as-is.

timofei-iatsenko · 2024-10-25T06:57:40Z

Thanks for your reviews and helps on this PR.

timofei-iatsenko commented Sep 26, 2024

View reviewed changes

packages/babel-core/src/transformation/index.ts Show resolved Hide resolved

nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Sep 30, 2024

nicolo-ribaudo reviewed Sep 30, 2024

View reviewed changes

nicolo-ribaudo added this to the v7.26.0 milestone Sep 30, 2024

timofei-iatsenko force-pushed the feat/async-pre-post branch from 9637819 to 295a9d7 Compare October 2, 2024 14:14

timofei-iatsenko requested a review from nicolo-ribaudo October 2, 2024 15:17

nicolo-ribaudo force-pushed the feat/async-pre-post branch from 3049bd1 to a448bbf Compare October 2, 2024 16:53

nicolo-ribaudo approved these changes Oct 2, 2024

View reviewed changes

nicolo-ribaudo added the pkg: core label Oct 2, 2024

timofei-iatsenko force-pushed the feat/async-pre-post branch from a448bbf to 3601be9 Compare October 3, 2024 10:23

timofei-iatsenko mentioned this pull request Oct 4, 2024

Modernizing the codebase lingui/js-lingui#2042

Closed

timofei-iatsenko added 2 commits October 20, 2024 12:27

feat: support async plugin's pre/post

22e893c

feat: add isAsync on PluginPass

166c7ff

timofei-iatsenko force-pushed the feat/async-pre-post branch from 3601be9 to 166c7ff Compare October 20, 2024 10:36

timofei-iatsenko commented Oct 20, 2024

View reviewed changes

nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Oct 23, 2024

Update types

710ce6c

JLHwung reviewed Oct 24, 2024

View reviewed changes

JLHwung approved these changes Oct 24, 2024

View reviewed changes

nicolo-ribaudo removed the PR: Needs Review A pull request awaiting more approvals label Oct 24, 2024

nicolo-ribaudo merged commit 0216853 into babel:main Oct 24, 2024

timofei-iatsenko deleted the feat/async-pre-post branch October 25, 2024 06:57

davidjbradshaw mentioned this pull request Oct 26, 2024

[Snyk] Upgrade @babel/runtime from 7.25.7 to 7.26.0 davidjbradshaw/iframe-resizer#1353

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 25, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2025

		await wait(50);
		this.file.ast.program.body[0].value = "success"

Uh oh!

Conversation

timofei-iatsenko commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

timofei-iatsenko commented Sep 30, 2024

Uh oh!

nicolo-ribaudo commented Sep 30, 2024

Uh oh!

timofei-iatsenko commented Oct 2, 2024

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timofei-iatsenko commented Oct 3, 2024

Uh oh!

nicolo-ribaudo commented Oct 9, 2024

Uh oh!

timofei-iatsenko commented Oct 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timofei-iatsenko commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

timofei-iatsenko commented Sep 26, 2024 •

edited

Loading

babel-bot commented Sep 26, 2024 •

edited

Loading