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.

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

Add concurrency support for more than one thread #681

Merged
merged 30 commits into from
Mar 4, 2021

Conversation

vicary
Copy link
Member

@vicary vicary commented Jan 13, 2021

What did you implement:

Adding concurrency support for more than one thread.

Conditionally supersedes #680 if release/5.4.0 is more preferable than master.

How did you implement it:

Please see #517 (comment), basically I did two changes:

  1. Replacing serizliedCompile: Boolean with concurrency: Number
  2. Translates serializedCompile: true to concurrency: 1 for backward compatibility

How can we verify it:

Use the config below when packaging:

package:
  individually: true

custom:
  webpack:
    concurrency: 5

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: Yes
Is it a breaking change?: No

@vicary vicary mentioned this pull request Jan 14, 2021
7 tasks
@coyoteecd
Copy link
Contributor

@miguel-a-calles-mba @j0k3r can you have a look and merge this? I am using vicary's fork for half a year and would love to switch back to the official package.

@j0k3r
Copy link
Member

j0k3r commented Jan 26, 2021

@coyoteecd we are in the process of cleaning up stuff to release 5.4.0 and this will be part of it

@j0k3r j0k3r closed this Jan 29, 2021
@j0k3r j0k3r deleted the branch serverless-heaven:master January 29, 2021 14:25
@coyoteecd
Copy link
Contributor

@j0k3r I don't understand what's going on here. Was this PR closed accidentally? Because I don't see it merged anywhere, and #680 is closed without being merged as well.

@j0k3r j0k3r reopened this Jan 29, 2021
@j0k3r j0k3r changed the base branch from release/5.4.0 to master January 29, 2021 19:13
@j0k3r
Copy link
Member

j0k3r commented Jan 29, 2021

Sorry I made a mistake while merging release/5.4.0 into master and removed it. The removal automatically closed that PR. It should now be rebased against the master and fix conflicts.

@vicary
Copy link
Member Author

vicary commented Jan 31, 2021

Not sure if I'm doing it wrong, but I found myself learning the backstory every conflict or two in the process of rebasing. This takes a day or two, could only do this on weekends.

@vicary
Copy link
Member Author

vicary commented Mar 1, 2021

@j0k3r During my rebase I found that the current master has these patterns.

// tests/packageModules.test.js
        return BbPromise.each([ '1.18.1', '2.17.0', '10.15.3' ], version => {
          getVersionStub.returns(version);
          return expect(module.packageModules()).to.be.fulfilled.then(() => BbPromise.all([]));
        }).then(() =>
          BbPromise.each([ '1.17.0', '1.16.0-alpha', '1.15.3' ], version => {
            getVersionStub.returns(version);
            return expect(module.packageModules()).to.be.fulfilled.then(() => BbPromise.all([]));
          })
        );

It has a strong code-smell here .then(() => BbPromise.all([])); which should either not there, or some tests are missed during rebase.

Not meant to make things more complicated than it should, please feel free to ignore this if it's intended.

I'll be rebasing to master as-is, just a friendly heads-up.

j0k3r and others added 3 commits March 1, 2021 13:04
Mostly to fix security issue but also to keep them updated.

Also:
- fix an ESLint error: `8:16  error  Do not access Object.prototype method 'hasOwnProperty' from target object  no-prototype-builtins`
- run prettier (following update)
- update `format` script with `$(pwd)` to fix issue with higher version of ESLint (>5)
- prettier now run on every js files
* Add copyExistingArtifacts to packageModules

* Refactor packageModules

* Set service path

* Generate artifact name from service

* Output artifacts to .webpack before copying

* Set artifact name for service packaging

* Skip webpack:compile if --no-build is set

* Add webpack:package:copyExistingArtifacts hook

* Make packageModules & packExternalModules no-op if skipCompile is set

* Refactor packageModules

* Remove artifact location setting from packageModules

* Update cleanup to check this.keepOutputDirectory

* Fix path join on Windows

Co-authored-by: Miguel A. Calles MBA <44813512+miguel-a-calles-mba@users.noreply.github.com>
@vicary vicary force-pushed the feat/concurrency-5.4.0 branch from 150b1a8 to 9f6e3d2 Compare March 1, 2021 11:54
@vicary
Copy link
Member Author

vicary commented Mar 1, 2021

The rebase is finished, this should be ready for merging.

lib/Configuration.js Outdated Show resolved Hide resolved
@j0k3r
Copy link
Member

j0k3r commented Mar 3, 2021

Awesome job for the rebase @vicary 👏

@j0k3r j0k3r added this to the 5.4.0 milestone Mar 3, 2021
@j0k3r j0k3r changed the title feat/concurrency@5.4.0 Add concurrency support for more than one thread Mar 3, 2021
Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Also, I think some tests should be updated as well about { concurrency: Infinity }. Don't you think?

lib/validate.js Outdated Show resolved Hide resolved
@vicary
Copy link
Member Author

vicary commented Mar 4, 2021

Also, I think some tests should be updated as well about { concurrency: Infinity }. Don't you think?

I agree that using Infinity may causes confusions. In fact compile.test.js is largely testing if the compiler has even run once, concurrency config there is mostly for cosmetic reasons.

To reduce ambiguity, how about we change the tests to use 1 instead?

Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Good job!

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.

6 participants