Support ember-concurrency v4

[jakesjews]

I used an unshift for adding the babel-plugin in case coverage is added later

[jelhan]

Thanks a lot for working on this!

I'm a little bot concerned on test coverage. I feel we should have a test that it's working as expected without any explicit ember-concurrency configuration in the consuming app. Maybe we can just remove the Babel plugin configuration from the test app?

Not sure of changes in ember-concurrency justify an ember try scenario for v3. @simonihmig @SanderKnauff Any thoughts on that?

[jelhan]

Tests for docs app is failing:

Error: Assertion Failed: It appears you're attempting to use the new task(async () => { ... }) syntax, but the async arrow task function you've provided is not being properly compiled by Babel.

This might indicate that it's not worked as expected for consuming apps which aren't configuring the babel plugin explicitly.

[SanderKnauff]

Not sure of changes in ember-concurrency justify an ember try scenario for v3. @simonihmig @SanderKnauff Any thoughts on that?

We only seem to be using it in the carousel, so I don't think it's worth that much.

Tests for docs app is failing:
Error: Assertion Failed: It appears you're attempting to use the new task(async () => { ... }) syntax, but the async arrow task function you've provided is not being properly compiled by Babel.
This might indicate that it's not worked as expected for consuming apps which aren't configuring the babel plugin explicitly.

The docs app does not explicitly declare ember-concurrency as a dependency. It does give a good indication that something it still not going 100% fine. @jakesjews, does this also happen when you try to build / test the docs app locally?

[jakesjews]

@SanderKnauff yeah they're failing I'll see what is up with it.

[jelhan]

Can we please remove this? The test app does not use ember-concurrency directly at all.

Suggested change

	babel: {
	plugins: [
	require.resolve('ember-concurrency/async-arrow-task-transform'),
	],
	},

I think we can even remove the devDependency on ember-concurrency in the test app. If we want to ensure a specific version being used to run the tests against, we could use pnpm.overrides instead. But as that is not only true for ember-concurrency but for many other dependencies as well, let's discuss that in a separate PR.

[jakesjews]

no problem. I removed it from the test app

[jelhan]

@jakesjews The tests for the docs app are failing again. Also Embroider tests are started to fail. I restarted 3 times to understand if it's flickering. But it was failing reproducible. Looks like one of the latest changes has broken it again.

[jakesjews]

@jelhan the good news is I finally got this passing. The bad news is the only way I could do it was by using a peer dependency on v4 like in Power Select so that would be a breaking change. I’ll can try some tricks with a wider peerDependency range but that would be a breaking change too.

[jelhan]

Thanks a lot for following up on it!

It seems that we are only using ember-concurrency in one component (<Be carousel>). I wonder if dropping the dependency might be an alternative to introducing a breaking change and requiring another peer dependency. I haven't looked into the complexity of such a refactoring yet though.

@SanderKnauff @simonihmig What are your thoughts about it?

[SanderKnauff]

Thanks a lot for following up on it!

It seems that we are only using ember-concurrency in one component (<Be carousel>). I wonder if dropping the dependency might be an alternative to introducing a breaking change and requiring another peer dependency. I haven't looked into the complexity of such a refactoring yet though.

@SanderKnauff @simonihmig What are your thoughts about it?

I found that the carousel is quite sensitive with it comes to timing. It would be nice if we could get rid of ember-concurrency if it is really that big of an issue, but I am afraid that getting the carousel to work correctly will take quite some effort.

[jelhan]

Can we continue using ember-concurrency but avoid the async-arrow notation? I haven't found anything in ember-concurrency docs on that one. But it sounds as if async-arrow notiation is just syntactic sugar. If we are writing the native format directly, we would avoid the need of the Babel transform.

Another alternative would be delaying ember-concurrency support until we refactored to a v2 addon. Many options on the table. 😄

[SanderKnauff]

I've just tried using this branch and... we can!
If we replace the foo = task(options, async () =>{}) syntax with @task(options) *foo() {} and simply change the awaits inside the function to yield, we might have support without needing the transform.

@jakesjews would you be open to removing the transform from the projects and fixing the bs-carousel by replacing the ember-concurrency task functions with generator functions?
After that, we should have wide support for ember-concurrency, including v4.

[jelhan]

Something seems to break for Embroider in fully optimized mode:

 ERROR in ./helpers/cancel-all.js 1:0-209
Module not found: Error: Can't resolve '../../node_modules/.pnpm/ember-concurrency@4.0.2_@babel+core@7.24.0_@glimmer+tracking@1.1.2_@glint+template@1.3.0_ember-source@5.7.0/node_modules/ember-concurrency/helpers/cancel-all' in '$TMPDIR/embroider/070db5/test-app/helpers'
 @ ./assets/test-app.js 261:13-48

ERROR in ./helpers/perform.js 1:0-206
Module not found: Error: Can't resolve '../../node_modules/.pnpm/ember-concurrency@4.0.2_@babel+core@7.24.0_@glimmer+tracking@1.1.2_@glint+template@1.3.0_ember-source@5.7.0/node_modules/ember-concurrency/helpers/perform' in '$TMPDIR/embroider/070db5/test-app/helpers'
 @ ./assets/test-app.js 288:13-45

ERROR in ./helpers/task.js 1:0-203
Module not found: Error: Can't resolve '../../node_modules/.pnpm/ember-concurrency@4.0.2_@babel+core@7.24.0_@glimmer+tracking@1.1.2_@glint+template@1.3.0_ember-source@5.7.0/node_modules/ember-concurrency/helpers/task' in '$TMPDIR/embroider/070db5/test-app/helpers'
 @ ./assets/test-app.js 297:13-42

3 errors have detailed information that is not shown.