Skip to content
This repository has been archived by the owner on Jun 24, 2024. It is now read-only.

COREWEB-41 - Offload bundling #100

Merged
merged 3 commits into from
Dec 18, 2017
Merged

COREWEB-41 - Offload bundling #100

merged 3 commits into from
Dec 18, 2017

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Dec 12, 2017

JIRA Issue

COREWEB-41

Status

READY

Description

We want to offload bundling to another thread/process to not lock up resources on potentially CPU heavy work.

As we use promises, worker-farm is not an option (it only supports callbacks), so I chose to go with jest-worker, which is what runs Jest tests in parallel.

I have not added new tests, as the observable behaviour is identical, beyond some extra needed cleanup work. We don't have programmatic API documented in the README, perhaps we should?

Todos

  • Tests
  • Documentation

Deploy Notes

N/A

Related PRs

  • None

Copy link
Member

@digitalsadhu digitalsadhu left a comment

Choose a reason for hiding this comment

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

Super nice.

Are there any possible complications or issues with using workers that we should test for?

Trying to think of edge cases like bundling the same bundle several times, multiple bundlings simultaneously etc.

Theres probably not super much so long as we trust jest-worker and its test coverage.

package.json Outdated
@@ -38,6 +38,7 @@
"convict": "^4.0.1",
"cors": "^2.8.4",
"express": "^4.16.2",
"jest-worker": "^21.3.0-beta.14",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

22.0.0 is coming any day now, will hold on merging until then

Copy link
Member

Choose a reason for hiding this comment

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

whats the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No code difference, I just dislike beta dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will be a code difference, actually: jestjs/jest#5068

@SimenB
Copy link
Contributor Author

SimenB commented Dec 12, 2017

Trying to think of edge cases like bundling the same bundle several times, multiple bundlings simultaneously etc.

We still await at the same spot, the only thing different is that we might queue things up there, instead of having the client nag us until we can handle the request. We now await an operation in another process instead of the same thread, control flow should be the exact same

@SimenB SimenB merged commit cf9742f into master Dec 18, 2017
@SimenB SimenB deleted the offload-bundling branch December 18, 2017 14:21
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants