Support package.include and package.exclude #327

janicduplessis · 2018-02-26T09:22:11Z

What did you implement:

Closes #394

This makes sure to respect the serverless package.include and package.exclude options when bundling.

How did you implement it:

Simple port of the packaging code that handles these option in serverless (https://github.com/serverless/serverless/blob/ba979a7a8fc6b095910a2cd67cb6413ac7524952/lib/plugins/package/lib/packageService.js#L130).

Also changed the glob dependency for globby since it's what serverless is using and made the port easier (supports an array of glob). It was only used at one other place in the project.

How can we verify it:

Tested the change in a project by ignoring source maps generated by webpack (package.exclude: "*.map") and verifying the resulting zip did not contain the files. Also tested that it still works like before when not setting any of these options.
Added a test that asserts the correct glob pattern is passed.

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, debatable since the option was ignored before but I don't think so.

HyperBrain · 2018-02-26T09:54:12Z

lib/validate.js

-      const files = glob.sync(`${fileName}.*`, {
-        cwd: this.serverless.config.servicePath,
+      const files = globby.sync(`${fileName}.*`, {
+        cwd: this.serverless.config.servicePath || process.cwd(),


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.

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.

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

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

HyperBrain · 2018-02-26T09:57:08Z

Hi @janicduplessis . Thanks for the PR.

The packager works on the compiled output, which is built and defined by webpack. Excluding or including stuff there might break integrity. Including special files can be easily done the webpack-way by using the CopyFiles plugin.

Can you elaborate on the concrete use case that is solved by enabling the package includes and excludes? Maybe I just miss the use-case here 😄

janicduplessis · 2018-02-26T10:07:09Z

@HyperBrain Hello! The concrete use case for me is excluding source maps from the generated bundle. I still want webpack to generate them so I can upload them to crash reporting services but I don't want to add to the function size.

I assumed this option could solve this problem cleanly while still remaining somewhat generic. I agree we operate on compiled output there so it is a bit different than how it's use normally but it seems to work well to ignore specific file types. AFAIK the option isn't and couldn't be used for something else so I feel like it makes sense to reuse it here.

homerjam · 2018-02-26T18:55:03Z

Hi both, I've just come across a reason this PR would be useful. I'm using docker to npm rebuild linux bindings and need to include these explicitly using the package:include: option in serverless.yml. I'll be trying the CopyFiles plugin as an alternative for now.

Thanks

HyperBrain · 2018-02-27T10:29:58Z

@janicduplessis @homerjam I though about the package include and exclude again. Reusing the properties from Serverless might be super confusing, because in the plugin's context they will only operate on the compiled code, but not on the sources. For webpack, the right way to include files from outside of the compile output directories is to use proper webpack plugins, like the file-loader or the copyfiles plugin.

However I think that the settings make sense and should be there for the compiled code.

What, if we put the settings under a new custom: webpack: package: property, to make visible that it is something different? I already had the plan to consolidate all webpack specific configuration settings under the custom: webpack: hive anyway.

homerjam · 2018-03-01T10:32:23Z

Sounds like a good idea to me. To be honest I think it'd help just to be clearer - I ended up here because I was confused why my includes weren't being included all of a sudden, having added serverless-webpack to an existing project. Now I understand it makes total sense!

homerjam · 2018-03-01T10:36:57Z

As an aside what do you think of including something like this? As this plugin is installing from npm (right?) it could go one further and build the bindings for linux too (if docker is available)?

HyperBrain · 2018-03-14T16:32:41Z

Info: I'll come back to this one soon after the 5.1.0 release.

HyperBrain · 2018-03-14T20:15:47Z

@homerjam To the aside you mentioned. Now with version 5.x having full support for arbitrary packagers, a Docker based one that would just use Docker to package the external dependencies would be possible. However this should be discussed in a separate issue, to see what others think about that and how exactly it should be implemented to fulfill the needs.

janicduplessis · 2018-04-03T04:44:48Z

@HyperBrain Sorry about taking so long to get back on this, I moved the config to a separate field, let me know if that looks good and all that's left to do is update the doc.

HyperBrain · 2018-04-03T10:27:02Z

lib/packageModules.js

@@ -34,11 +34,27 @@ function zip(directory, name) {

  const output = fs.createWriteStream(artifactFilePath);

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


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.

HyperBrain · 2018-04-03T10:30:06Z

tests/packageModules.test.js

@@ -319,6 +319,56 @@ describe('packageModules', () => {
      });
    });

+    it('should respect package.include and package.exclude', () => {
+      _.set(module, 'configuration.package', {


Same here accordingly: _.set(module, 'configuration._config.package', {

stevemao · 2019-05-08T05:37:08Z

Would be good if package.include works the same way as CopyFiles plugin. For people just added this plugin they don't need to add CopyFiles plugin and it still works the same way as before.

xyklex · 2019-09-11T20:45:18Z

What is left for this feature to be merged, only the changes @HyperBrain mentioned?

jpoissant · 2020-05-11T13:32:39Z

Any news on this merge?

miguel-a-calles-mba

Hi, may you please address the merge conflicts? Thanks.

simon-casasoft · 2021-04-16T11:54:35Z

Any update on this please?

nponeccop · 2021-04-16T14:23:50Z

There are merge conflicts. The PR will be merged once someone resolves them. Anyone can do it. I will do it once I have time.

janicduplessis · 2021-08-05T17:43:57Z

I don't think this is needed anymore, and no longer have a use case for it. If this is still useful to someone feel free to pickup this PR.

janicduplessis force-pushed the include-exclude branch from a148b34 to 9bec03e Compare February 26, 2018 09:36

HyperBrain requested changes Feb 26, 2018

View reviewed changes

Support package.include and package.exclude

63671e5

janicduplessis force-pushed the include-exclude branch from 9bec03e to 63671e5 Compare April 3, 2018 01:10

Move to custom config

29b6f75

janicduplessis force-pushed the include-exclude branch from 9096c94 to 29b6f75 Compare April 3, 2018 04:26

HyperBrain requested changes Apr 3, 2018

View reviewed changes

HyperBrain mentioned this pull request May 15, 2018

Exclude Readme and tests files in external modules #394

Open

hassankhan added the feature label Apr 30, 2019

miguel-a-calles-mba added this to the 5.4.0 milestone May 13, 2020

miguel-a-calles-mba reviewed May 13, 2020

View reviewed changes

miguel-a-calles-mba changed the base branch from master to release/5.4.0 May 13, 2020 14:40

j0k3r removed this from the 5.4.0 milestone Nov 20, 2020

j0k3r changed the base branch from release/5.4.0 to master November 20, 2020 20:17

janicduplessis closed this Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support package.include and package.exclude #327

Support package.include and package.exclude #327

janicduplessis commented Feb 26, 2018 •

edited by HyperBrain

HyperBrain Feb 26, 2018

janicduplessis Feb 26, 2018

HyperBrain Feb 26, 2018

HyperBrain Feb 26, 2018 •

edited

HyperBrain commented Feb 26, 2018 •

edited

janicduplessis commented Feb 26, 2018

homerjam commented Feb 26, 2018

HyperBrain commented Feb 27, 2018

homerjam commented Mar 1, 2018

homerjam commented Mar 1, 2018

HyperBrain commented Mar 14, 2018

HyperBrain commented Mar 14, 2018

janicduplessis commented Apr 3, 2018

HyperBrain Apr 3, 2018 •

edited

HyperBrain Apr 3, 2018

stevemao commented May 8, 2019

xyklex commented Sep 11, 2019

jpoissant commented May 11, 2020

miguel-a-calles-mba left a comment

simon-casasoft commented Apr 16, 2021

nponeccop commented Apr 16, 2021

janicduplessis commented Aug 5, 2021

Support package.include and package.exclude #327

Support package.include and package.exclude #327

Conversation

janicduplessis commented Feb 26, 2018 • edited by HyperBrain

What did you implement:

How did you implement it:

How can we verify it:

Todos:

HyperBrain Feb 26, 2018

Choose a reason for hiding this comment

janicduplessis Feb 26, 2018

Choose a reason for hiding this comment

HyperBrain Feb 26, 2018

Choose a reason for hiding this comment

HyperBrain Feb 26, 2018 • edited

Choose a reason for hiding this comment

HyperBrain commented Feb 26, 2018 • edited

janicduplessis commented Feb 26, 2018

homerjam commented Feb 26, 2018

HyperBrain commented Feb 27, 2018

homerjam commented Mar 1, 2018

homerjam commented Mar 1, 2018

HyperBrain commented Mar 14, 2018

HyperBrain commented Mar 14, 2018

janicduplessis commented Apr 3, 2018

HyperBrain Apr 3, 2018 • edited

Choose a reason for hiding this comment

HyperBrain Apr 3, 2018

Choose a reason for hiding this comment

stevemao commented May 8, 2019

xyklex commented Sep 11, 2019

jpoissant commented May 11, 2020

miguel-a-calles-mba left a comment

Choose a reason for hiding this comment

simon-casasoft commented Apr 16, 2021

nponeccop commented Apr 16, 2021

janicduplessis commented Aug 5, 2021

janicduplessis commented Feb 26, 2018 •

edited by HyperBrain

HyperBrain Feb 26, 2018 •

edited

HyperBrain commented Feb 26, 2018 •

edited

HyperBrain Apr 3, 2018 •

edited