cucumber / cucumber-js

Cucumber for JavaScript
https://cucumber.io
MIT License
5.04k stars 1.09k forks source link

Issue 2262 ES imports in parallel #2263

Open michael-lloyd-morris opened 1 year ago

michael-lloyd-morris commented 1 year ago

🤔 What's changed?

Optimization in the support.ts file to gather the dynamic import Promises into an array and then resolve them with a single Promise.all() call.

⚡️ What's your motivation?

This should yield a noticeable speedup on Cucumber installs with very large ES step definition libraries. Other Cucumber installs will be unaffected.

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

If there are other instances in the codebase where an await is inside a loop then this pattern should be applied there as well.

📋 Checklist:

coveralls commented 1 year ago

Coverage Status

Coverage: 98.561% (+0.0006%) from 98.561% when pulling 5a9d473b50ded1fcbdc4c8e019fab7cb2ba7bc2b on michael-lloyd-morris:issue-2262-es-parallel into 627030d4d657b7efc19b1343d3d77d77c774139e on cucumber:main.

michael-lloyd-morris commented 1 year ago

There is a behavior change in that the step files resolve simultaneously, but I can't imagine it mattering because Cucumber never gave the end users a way to control the order their step files are resolved in. This might be a concern so I sharded this optimization off to it's own PR for review.

michael-lloyd-morris commented 1 year ago

@davidjgoss raised the important point that there may be code out there relying on this sequential load behavior. I'm not so sure this is the case, but that is the safest approach to this optimization.

That would mean delaying this to the next major release and/or using a cli or config flag to allow opt in.

I'm inclined to follow David's advice and just defer to the next major, especially since require imports are going to remain sequential as that is the nature of the beast. Also, the optimization isn't worth the headache except perhaps for the very largest of test libraries.

davidjgoss commented 1 year ago

To be clear I do think this is the right thing to do. With require being synchronous there was never a way to load in parallel but it's one of the big pluses of ESM so we should lean into it. The potential non-deterministic order of Before etc hooks running could be a surprise to some users so I think we should do it in a major to be cautious. Our advice to users wanting to ensure consistent order of hooks running should be to have them in one file.

michael-lloyd-morris commented 1 year ago

Agreed, so flag for version 10 and I'll come back to this when work on that major starts to catch up anything that fell behind? How do I do that or do you want to do it?

davidjgoss commented 11 months ago

Noting that this would, as it stands, break the parallel runtime because that depends on support code being registered in a consistent order across coordinator and workers. We'll need to try and find another way around that in order to go ahead with this change.