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

fix excludeRegex by actually excluding files #777

Closed
wants to merge 2 commits into from

Conversation

l1b3r
Copy link

@l1b3r l1b3r commented Apr 8, 2021

What did you implement:

Closes #776

How did you implement it:

  • When querying a directory for files using glob, add a nodir: true parameter to have only actual files in the resulting files array.
  • Filter the files using excludeRegex option if specified. If verbose, log the amount of excluded files.
  • Then, when zipping files with bestzip, pass files instead of "*" so that only specified files end up in the zip.

How can we verify it:

webpack.config.js

module.exports = {
    mode: 'production',
    target: 'node',
    devtool: 'source-map',
    resolve: {
        extensions: ['.mjs', '.ts', '.js']
    },
    output: {
        libraryTarget: 'commonjs2',
        path: path.join(__dirname, '.webpack'),
        filename: '[name].js'
    },
    externals: /^aws-sdk(\/.*)?/,
    // Place all node_modules in a separate vendor chunk
    optimization: {
        minimize: false,
        splitChunks: {
            chunks: 'all',
            maxInitialRequests: Infinity,
            minSize: 0,
            cacheGroups: {
                node_modules: {
                    test: /[\\/]node_modules[\\/]/,
                    name: 'vendor'
                }
            },
        },
    },
    module: {
        rules: [
            {
                test: /\.ts$/,
                exclude: /node_modules/,
                loader: 'ts-loader'
            }
        ]
    }
}

excludeRegex: /.*\.map/

Before:

demo-project
│   vendor.js
│   vendor.js.map
│
└───src
    └───handlers
            viewer-request.js
            viewer-request.js.map

After:

demo-project
│   vendor.js
│
└───src
    └───handlers
            viewer-request.js

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

P.S. I had to add an additional missing undefined-check in lib/wpwatch.js to pass 3 failing unit tests.

@j0k3r
Copy link
Member

j0k3r commented Apr 9, 2021

Thanks for providing that fix.
In fact, it'll also solve #744 because your provide the files list to zip instead of using * (which does not include dotfiles by default).

But after testing the excludeRegex it seems to not work at all because the regex is a string and the test is wrong.
In the test tests/packageModules.test.js we are defining the regex as a real regex:

        module.configuration = new Configuration({
          webpack: {
            excludeRegex: /.*/
          }
        });

So the test is working.
Here is the dumped configuration:

Configuration {
  _config: {
    excludeRegex: /.*/,
    webpackConfig: 'webpack.config.js',
    includeModules: false,
    packager: 'npm',
    packagerOptions: {},
    keepOutputDirectory: false,
    config: null,
    concurrency: 4
  },
  _hasLegacyConfig: false
}

But when defining the regex in serverless.yml the regex is interpreted as a string (thanks for yaml...).
So the test should define the configuration as:

        module.configuration = new Configuration({
          webpack: {
            excludeRegex: '/.*/'
          }
        });

And in that case, test are failing.
Here is the dumped configuration:

Configuration {
  _config: {
    excludeRegex: '/.*/',
    webpackConfig: 'webpack.config.js',
    includeModules: false,
    packager: 'npm',
    packagerOptions: {},
    keepOutputDirectory: false,
    config: null,
    concurrency: 4
  },
  _hasLegacyConfig: false
}

I tried to use RegExp as a workaround without success yet:

new RegExp(this.configuration.excludeRegex)

Do you have an idea on how to fix it?

@j0k3r
Copy link
Member

j0k3r commented Apr 9, 2021

As you created your PR on your master branch, I can't force push on it. So I created an other PR, cherry-picked your commit and applying some changes. Go check it #780

@j0k3r j0k3r closed this Apr 9, 2021
@l1b3r
Copy link
Author

l1b3r commented Apr 9, 2021

@j0k3r Yeah, sorry I didn't pay enough attention to that. I think you should omit the slashes around the actual regular expression if you declare it as a string in serverless.yml.

Then you can pass this string into String.prototype.match and it will do the automatic conversion of this string to aRegExp object under the hood.

So, just define the excludeRegex as ".*" and the rest should work.

I can do a separate PR with these changes if you'd like.

@j0k3r
Copy link
Member

j0k3r commented Apr 9, 2021

I finally did it in #780, can you have a look?

@pveller
Copy link

pveller commented May 5, 2021

when zipping files with bestzip, pass files instead of "*" so that only specified files end up in the zip.

I started getting spawn E2BIG on sls package in bestzip where it shells out (nativeZip()) with a very long command line that I would expect to be very different if it was actually using * instead? All the individual files that serverless need to pack in my list of dependencies go over 2x of allowed ARG_MAX.

Downgrade to 5.4.0 solved the problem. I will post a separate issue to make it easier to track.

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.

excludeRegex not excluding files
3 participants