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

Jump to bottom

Support package.include and package.exclude #327

Closed
wants to merge 2 commits into from
Closed

Support package.include and package.exclude #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63671e5
Support package.include and package.exclude
janicduplessis Feb 26, 2018
29b6f75
Move to custom config
janicduplessis Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 18 additions & 2 deletions lib/packageModules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const _ = require('lodash');
const path = require('path');
const archiver = require('archiver');
const fs = require('fs');
const glob = require('glob');
const globby = require('globby');
const semver = require('semver');

function setArtifactPath(funcName, func, artifactPath) {
Expand Down Expand Up @@ -34,11 +34,27 @@ function zip(directory, name) {

const output = fs.createWriteStream(artifactFilePath);

const files = glob.sync('**', {
const exclude = _.get(this.configuration, 'package.exclude', []);
Copy link
Member

@HyperBrain HyperBrain Apr 3, 2018 •
edited

Choose a reason for hiding this comment

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

The configuration is now separated into the Configuration object. You should add a getter for package and set viable defaults (empty arrays) there and add some unit tests. Then just use const { exclude, include } = this.configuration.package here instead of bypassing the integrity of the class.

const include = _.get(this.configuration, 'package.include', []);

const patterns = _.concat(
['**'],
_.map(exclude, pattern => {
if (pattern.charAt(0) !== '!') {
return `!${pattern}`;
} else {
return pattern.substring(1);
}
}),
include
);

const files = globby.sync(patterns, {
cwd: directory,
dot: true,
silent: true,
follow: true,
nodir: true,
});

if (_.isEmpty(files)) {
Expand Down
6 changes: 3 additions & 3 deletions lib/validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const BbPromise = require('bluebird');
const path = require('path');
const fse = require('fs-extra');
const glob = require('glob');
const globby = require('globby');
const lib = require('./index');
const _ = require('lodash');
const Configuration = require('./Configuration');
Expand Down Expand Up @@ -31,8 +31,8 @@ module.exports = {
};

const getEntryExtension = fileName => {
const files = glob.sync(`${fileName}.*`, {
cwd: this.serverless.config.servicePath,
const files = globby.sync(`${fileName}.*`, {
cwd: this.serverless.config.servicePath || process.cwd(),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if defaulting to CWD is right here. It will miss the handlers in case CWD !== servicePath and render the webpack configuration invalid. In general, serverless-webpack depends on servicePath everywhere and uses it to create it's outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this since servicePath was null in some cases (maybe just in tests) and globby doesn't support that while glob did. I verified that both libs default to process.cwd if cwd is not set so this should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that can be the tests. Not all of them intialize a full config object for testing. So this should be ok 👍

Copy link
Member

Choose a reason for hiding this comment

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

BTW: I just remember another open PR, that stabilizes the directories and uses webpackconfig.context to keep the webpack root directory. So this line would be void if the two PRs are merged together.
#325

nodir: true
});

Expand Down