Skip to content
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

Update workerpool #203

Closed
wants to merge 6 commits into from
Closed

Update workerpool #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7de6ddc
update workerpool
mansona Jun 5, 2021
5ea8746
set workerType to 'process' as default was changed to thread
mansona Jun 5, 2021
5e533e2
fix assertions in tests
mansona Jun 5, 2021
355051d
Add GitHub Actions CI setup.
rwjblue Jun 12, 2020
8234774
Updating `.gitattributes` content.
rwjblue Oct 16, 2020
00ef732
Remove TravisCI and AppVeyor
rwjblue Oct 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,5 @@
* text eol=lf
# Set the default behavior, in case people don't have core.autocrlf set.
* text=auto

# JS files should always have LF line endings on checkout.
*.js text eol=lf
48 changes: 48 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
name: CI

on:
push:
branches:
- master
- 'v*' # older version branches
tags:
- '*'

pull_request: {}
schedule:
- cron: '0 6 * * 0' # weekly, on sundays

jobs:
test:
name: "Node ${{ matrix.node }} - ${{ matrix.os }}"
runs-on: "${{matrix.os}}-latest"

strategy:
matrix:
os: ['ubuntu', 'windows', 'macOS']
node: ['10', '12', '14']
Copy link
Member

Choose a reason for hiding this comment

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

Doesn’t test node 16 (which is theoretically the point?)

Copy link
Author

Choose a reason for hiding this comment

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

Yes you're right I should have added that. I'll split the CI stuff and then add 16 in this PR when it's rebased 👍


steps:
- uses: actions/checkout@v1
- uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node }}
- name: install dependencies
run: yarn install --frozen-lockfile
- name: test
run: yarn test

floating-test:
name: Floating dependencies
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v1
- uses: actions/setup-node@v1
with:
node-version: '14.x'
- name: install dependencies
run: yarn install --no-lockfile
- name: test
run: yarn test

36 changes: 0 additions & 36 deletions .travis.yml
View file
Open in desktop

This file was deleted.

29 changes: 0 additions & 29 deletions appveyor.yml
View file
Open in desktop

This file was deleted.

5 changes: 4 additions & 1 deletion lib/parallel-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ function getWorkerPool() {
if (existingPool) {
pool = existingPool;
} else {
pool = workerpool.pool(path.join(__dirname, 'worker.js'), { maxWorkers: JOBS });
pool = workerpool.pool(path.join(__dirname, 'worker.js'), {
maxWorkers: JOBS,
workerType: 'process'
Copy link
Member

Choose a reason for hiding this comment

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

Why force process here? If thread is available I don’t see why we’d want to force process.

Copy link
Author

Choose a reason for hiding this comment

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

well tests fail if we don't 😂 I don't really understand why, I was trying to do the minimum effort to get this passing

Copy link
Member

@stefanpenner stefanpenner Jun 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

thread is the preferred approach here... fallback to process only when absolutely needed.

});
process[globalPoolID] = pool;
}
return pool;
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"heimdalljs-logger": "^0.1.7",
"json-stable-stringify": "^1.0.0",
"rsvp": "^4.8.2",
"workerpool": "^2.3.0"
"workerpool": "^6.1.4"
Copy link
Member

Choose a reason for hiding this comment

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

This is breaking in a way that we typically do not do... 🤔

Specifically, this is changing an old major to drop support for a Node major version with the expectation of releasing without a breaking change indication. This is likely to cause exactly the kind of breakage you are trying to fix (introduced CI failures).

Copy link
Author

Choose a reason for hiding this comment

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

So I wasn't intending to drop support for any node version 🤔 the idea was that the tests should still check the correct Node version. I guess technically because workerpool aren't testing against our supported Node versions they could break older ones at any time, is that what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I guess technically because workerpool aren't testing against our supported Node versions they could break older ones at any time, is that what you mean?

Yes, exactly. They could update to (for example) async/await syntax, which would be breaking for Node 8 users (which ember-cli-babel@6 still supports).

Copy link
Member

Choose a reason for hiding this comment

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

IMO, the right fix (as I mentioned in #203 (review)) is to disable parallelizing when running on Node 16. It is the least invasive change, and has the least likely hood of breaking folks.

},
"devDependencies": {
"amd-name-resolver": "1.2.0",
Expand Down
4 changes: 2 additions & 2 deletions tests/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,7 +961,7 @@ describe('on error', function() {
expect.fail('', '', 'babel should throw an error');
},
function onFailure(err) {
expect(err.message).to.eql('Worker terminated unexpectedly');
expect(err.message).to.include('Workerpool Worker terminated Unexpectedly');
}
);
});
Expand Down Expand Up @@ -1377,7 +1377,7 @@ describe('buildFromParallelApiInfo()', function() {
expect.fail('', '', 'should have thrown an error');
}
catch (err) {
expect(err.message).to.eql("Cannot find module 'some/file/that/does/not/exist'");
expect(err.message).to.include("Cannot find module 'some/file/that/does/not/exist'");
}
});

Expand Down
11 changes: 5 additions & 6 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -941,7 +941,7 @@ number-is-nan@^1.0.0:
version "1.0.1"
resolved "https://registry.npmjs.org/number-is-nan/-/number-is-nan-1.0.1.tgz#097b602b53422a522c1afb8790318336941a011d"

object-assign@4.1.1, object-assign@^4.1.0:
object-assign@^4.1.0:
version "4.1.1"
resolved "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz#2109adc7965887cfc05cbbd442cac8bfbb360863"

Expand Down Expand Up @@ -1212,11 +1212,10 @@ wordwrap@~0.0.2:
version "0.0.3"
resolved "https://registry.npmjs.org/wordwrap/-/wordwrap-0.0.3.tgz#a3d5da6cd5c0bc0008d37234bbaf1bed63059107"

workerpool@^2.3.0:
version "2.3.0"
resolved "https://registry.yarnpkg.com/workerpool/-/workerpool-2.3.0.tgz#86c5cbe946b55e7dc9d12b1936c8801a6e2d744d"
dependencies:
object-assign "4.1.1"
workerpool@^6.1.4:
version "6.1.4"
resolved "https://registry.yarnpkg.com/workerpool/-/workerpool-6.1.4.tgz#6a972b6df82e38d50248ee2820aa98e2d0ad3090"
integrity sha512-jGWPzsUqzkow8HoAvqaPWTUPCrlPJaJ5tY8Iz7n1uCz3tTp6s3CDG0FF1NsX42WNlkRSW6Mr+CDZGnNoSsKa7g==

wrappy@1:
version "1.0.2"
Expand Down