Skip to content

Conversation

@kfranqueiro
Copy link
Contributor

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
Copy link

netlify bot commented May 16, 2025

Deploy Preview for respec-pr ready!

Name Link
🔨 Latest commit 251008e
🔍 Latest deploy log https://app.netlify.com/projects/respec-pr/deploys/68934afde7a1af0008bc5022
😎 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
Contributor 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.)

@marcoscaceres
Copy link
Contributor

@sidvishnoi, I don't think we should block on the timing issue... we should figure out some other way of dealing with it as issues come up.

@marcoscaceres marcoscaceres requested a review from Copilot August 6, 2025 06:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the execution order of pre-process and post-process functions to ensure they run sequentially rather than concurrently. The changes address issue #4961 by replacing the parallel execution pattern with sequential execution.

  • Replaces Promise.all() with for...of loops to execute async functions in order
  • Updates tests to verify both functionality and execution order
  • Adds a shared counter mechanism to track invocation sequence

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/spec/core/pre-process-spec.js Updates existing tests to use toContain and adds new test verifying execution order
tests/spec/core/pre-process-spec.html Introduces counter mechanism and updates functions to track execution order
src/core/pre-process.js Replaces parallel execution with sequential for...of loop
src/core/post-process.js Replaces parallel execution with sequential for...of loop

@sidvishnoi sidvishnoi merged commit 88a7fdf into speced:main Aug 6, 2025
11 checks passed
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.

preProcess and postProcess do not execute "in order" as documented for async functions

3 participants