8000 fix(core/pre-process, core/post-process): Run async functions in order by kfranqueiro · Pull Request #4962 · speced/respec · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(core/pre-process, core/post-process): Run async functions in order #4962

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(core/pre-process, core/post-process): Run async functions in order #4962

wants to merge 2 commits into from
+70 −59

Conversation

kfranqueiro
Copy link

Fixes #4961.

  • Updates pre/post-process tests to include verification of invocation order
    • Adds a counter shared between invocations that is incremented and output for each one
    • Changes existing toBe expectations to toContain for pass/fail without order
    • Adds new test with precise toBe expectations including order
  • Revises code flow of pre- and post-process to use for...of with an await on each iteration, instead of map that invokes every function up-front followed by Promise.all
    • Note: the changes to the plugins are easier to read with whitespace ignored (?w=1).

The new test fails when run without the fix commit.

Question: if this gets merged, would it also be acceptable for me to make an attempt at updating the preProcess and postProcess docs to mention that they support promises?

@netlify Netlify
Copy link
netlify bot commented May 16, 2025 •
edited
Loading

✅ Deploy Preview for respec-pr ready!

Name Link
🔨 Latest commit 64d7c1b
🔍 Latest deploy log https://app.netlify.com/projects/respec-pr/deploys/68273fecb0430b0008ef8df4
😎 Deploy Preview https://deploy-preview-4962--respec-pr.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@sidvishnoi
Copy link
Member

Thank you @kfranqueiro for filing the detailed bug report and sending this PR.

In general, the idea is correct (and wanted), except there's one problem: Each plugin (including preProcess, postProcess) in ReSpec has a timeout (15 seconds per plugin).
Assume there are 4 functions, each taking 5 seconds. We'll hit that timeout when running in serial. With Promise.all, that timeout won't be triggered in general.

A possible way will be to increase timeout of preProcess/postProcess functions, as an exception.
Another way I experimented in #3320 was to add each "plugin" (in this case, each preProcess/postProcess function) to the top-level plugins list on load. Then each preProcess/postProcess function will be treated as a plugin in itself, use 15s timeout, and run in serial.

@kfranqueiro
Copy link
Author

Thanks for the additional context. I take it #3320 never gained traction...

I was certainly a bit wary in terms of how much changing this might extend the time of pre- and post-process, depending on what's been done with them in various configurations. I almost wondered if I should somehow make this behavior optional, even if it is intended to align with current documentation and historical expectations for sync functions.

On the subject of making things optional, I also wonder if it'd be reasonable to make the timeouts for pre- and post-process in particular customizable. e.g., optionally support the following:

preProcess: {
  functions: [/* the usual goes here */],
  timeout: 30000
}

(If I were to make the fix itself optional it could be exposed similarly, via something like sequentialAsync.)

RE potentially extending the hard-coded timeout (either as an exception or across the board), I'm curious what motivated the timeouts, e.g. what plugins had a history of taking too long, and for what kinds of reasons? (I'm assuming network timeouts could be a common one.)

(I suppose another option could be to expose the 15000 via e.g. config.pluginTimeout.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

preProcess and postProcess do not execute "in order" as documented for async functions
2 participants
@kfranqueiro @sidvishnoi
0