Integration with deploy package function #130

franciscocpg · 2017-06-02T18:43:14Z

Fixes issue #107.
Also PR #110 seems incomplete and it looks like the author is not working on it anymore.
Here is a working webpack.config.js

const yaml = require('js-yaml');
const fs = require('fs');
const path = require('path');
const nodeExternals = require('webpack-node-externals');

const handlerRegex = /\.[_$a-zA-Z\xA0-\uFFFF][_$a-zA-Z0-9\xA0-\uFFFF]*$/;
const include = './_webpack/include.js';
const entries = {};

const doc = yaml.safeLoad(fs.readFileSync('serverless.yml', 'utf8'));

// If we pass a function, package just it 
// not all the functions listed on serverless.yml
const serverlessFunction = process.env.serverlessFunction;
const functions = serverlessFunction ?
  { serverlessFunction: doc.functions[serverlessFunction] }
  : doc.functions;
for (const key in functions) { // eslint-disable-line guard-for-in
  const handler = functions[key].handler;
  const entryKey = handler.replace(handlerRegex, '');
  entries[entryKey] = [include, `./${entryKey}.js`];
}

module.exports = {
  entry: entries,
  target: 'node',
  // Generate sourcemaps for proper error messages
  devtool: 'source-map',
  // Since 'aws-sdk' is not compatible with webpack,
  // we exclude all node dependencies
  externals: [nodeExternals()],
  // Run babel on all .js files and skip those in node_modules
  module: {
    loaders: [{
      test: /\.js$/,
      loaders: ['babel-loader'],
      include: __dirname,
      exclude: /node_modules/,
    }]
  },
  output: {
    libraryTarget: 'commonjs',
    path: path.join(__dirname, '.webpack'),
    filename: '[name].js'
  }
};

afhammad · 2017-06-08T15:55:11Z

Installed and tested this PR

Although Lambda shows this when opening the function on AWS console:

Your Lambda function "fnName" cannot be edited inline since the file name specified in the handler does not match a file name in your deployment package.

The function does get packaged correctly and work.

franciscocpg · 2017-06-30T15:22:43Z

@HyperBrain
Related to #134
This PR replaces your PR #110.
I have used it in some of my projects for about 1 month now without any problems.

HyperBrain · 2017-06-30T15:25:42Z

@franciscocpg Ok. Then I will close my PR.

HyperBrain · 2017-06-30T15:56:18Z

Regarding the new label please check my comment #134 (comment)

HyperBrain · 2017-06-30T16:13:00Z

@franciscocpg Can you check if #91 is also replaced, as it also about hooking package function and on the first sight does something quite similar?

franciscocpg · 2017-06-30T19:07:21Z

@HyperBrain. Yes it replaces.
In fact I've tested the branch from #91 and it's not working because the zip file is not being copied to .serverless folder.

HyperBrain · 2017-07-01T12:10:02Z

lib/validate.js

@@ -18,6 +18,9 @@ module.exports = {
        throw new this.serverless.classes
          .Error('The webpack plugin could not find the configuration file at: ' + webpackConfigFilePath);
      }
+      if (this.options.function) {
+        process.env.serverlessFunction = this.options.function;


Is there any reason why an additional environment variable is set here? I would not set anything in the process' environment as it is global. Better would be to set it as member of the plugin object with this.XXX = ... in case it is needed by the plugin itself.
If it is meant to be used in the vars of serverless.yml, you can reference it via options as ${opt:function} there, even with specifying a default in case it is not set (${opt:function, 'none'}).

@HyperBrain I'm using this env var with webpack.config.js. But reading further comments I realized that you guys have suggested a better approach.

HyperBrain · 2017-07-01T12:15:28Z

lib/copyFunctionArtifact.js

+        resolve();
+      });
+
+      readStream.pipe(fs.createWriteStream(artifactFilePath));


Can this call itself throw, or does it delegate any errors to the 'error' event in any case?
Otherwise the pipe call itself should be wrapped into a try/catch and call reject() in the catch block.

@HyperBrain pipe emits an error event so the better approach would be:

readStream.pipe(fs.createWriteStream(artifactFilePath)).on('error', (err) => { reject(err); });

Sounds reasonable 👍

johnf · 2017-07-01T23:34:19Z

I like this approach. I wonder though if we should provide a utility function. Something like

const slsw = require('serverless-webpack');

module.exports = {
  entry: slsw.entries(),
  // other bits
  output: {
    libraryTarget: 'commonjs',
    path: path.join(__dirname, '.webpack'),
    filename: '[name].js'
  }
};

HyperBrain · 2017-07-03T10:43:06Z

@johnf Interesting idea 🙌 . ~~As this is out of scope for this particular PR, would you mind creating a separate issue for the "library extension" of the plugin with a detailed description what problem it addresses? Thanks.~~
Sorry, my fault ;-) Yes, this would eliminate the transfer of the function entry point via environment variables and keep the implementation isolated within the plugin. The plugin could expose a lib export instead of entries() where we can integrate other library functions as soon as we need others.
Then it would be: entry: slsw.lib.entries - makes more sense as property getter for me.

@hassankhan Any thoughts on this? If we agree to have this extended in that way, we might delay the "deploy function" support to version 2.1.0, so that we can get the other things out in the meantime.

hassankhan · 2017-07-03T11:05:15Z

I think having exported entries is a pretty cool idea 👍 .

We (at work) don't really do single function deploys, so my experience is a bit limited 😅 . My only worry would be to inadvertently affect whole package deploys. That said, it would save a lot of trouble having to manually sync serverless.yml and webpack.config.js and I'd be up for pushing to v2.1 so we can test it properly.

HyperBrain · 2017-07-03T11:08:11Z

Ok. I'll tag it accordingly... we will do function deploys as soon as this is done 😄

HyperBrain · 2017-07-05T09:28:21Z

@franciscocpg Do you need any help or more information with the entries fix? I could create a separate branch in the repository to consolidate the feature, so that we can add this with a separate PR before it finds its way to master.

franciscocpg · 2017-07-05T12:39:49Z

Hi @HyperBrain
So if I don't misunderstood anything we are going to move this logic (with some modifications) from webpack.config.js

const entries = {};

const doc = yaml.safeLoad(fs.readFileSync('serverless.yml', 'utf8'));

// If we pass a function, package just it 
// not all the functions listed on serverless.yml
const serverlessFunction = process.env.serverlessFunction;
const functions = serverlessFunction ?
  { serverlessFunction: doc.functions[serverlessFunction] }
  : doc.functions;
for (const key in functions) { // eslint-disable-line guard-for-in
  const handler = functions[key].handler;
  const entryKey = handler.replace(handlerRegex, '');
  entries[entryKey] = [include, `./${entryKey}.js`];
}

to our plugin (a new file called index.js or lib.js) and then export entries (through lib expose) so one could use like this at webpack.config.js:

const slsw = require('serverless-webpack');
module.exports = {
  entry: slsw.lib.entries,
  // other bits
  output: {
    libraryTarget: 'commonjs',
    path: path.join(__dirname, '.webpack'),
    filename: '[name].js'
  }
};

Did I understand correctly?

HyperBrain · 2017-07-05T12:44:16Z

I understood it that way 😄 . The webpack config should have no knowledge of Serverless internals, so everything that interacts directly with the plugin or Serverless should be in the plugin.

After a solution is finsihed we also should test the behavior with all possible configurations (may it be the entries retrieved from the plugin or statically set entries).

franciscocpg · 2017-07-20T18:02:16Z

Sorry for the late, guys. I was very busy these days.
I made the changes we've discussed here and also I've updated examples folder.
IMO babel-dynamically-entries (entries retrieved from the plugin) and babel-multiple-statically-entries (statically set entries) examples are a good place to do some manual tests for this PR (sls deploy, sls deploy function -f first, sls deploy function -f second, sls webpack invoke -f first, sls webpack invoke -f second).
I think this PR is still a WIP because I didn't add any specs to tests (hope to do this ASAP) but it looks like everything is working as expected.

HyperBrain · 2017-07-20T18:04:36Z

@franciscocpg Great stuff and a big thank you 🙌 . I will test it over the weekend and review the PR again.

HyperBrain · 2017-07-24T21:50:30Z

Yes, given that your fork's master is up-to-date with the webpack's master 👍 .
But first remove the merge commit with "reset"

HyperBrain · 2017-07-24T21:52:03Z

According to the git docs (https://git-scm.com/docs/git-rebase) it should be git rebase --onto master or as you said git rebase master

HyperBrain · 2017-07-24T22:01:51Z

I managed to rebase the commits locally. If you agree, I can commit them back to your branch into the PR.

franciscocpg · 2017-07-24T22:02:37Z

Ok @HyperBrain

HyperBrain · 2017-07-24T22:06:53Z

Can you check if "enable edits by maintainers" is checked in the PR?

franciscocpg · 2017-07-24T22:09:02Z

I've just checked Allow edits from maintainers now

HyperBrain · 2017-07-24T22:14:01Z

It worked 💯

You can now pull the branch again locally. It is now rebased onto the current master.

HyperBrain · 2017-07-24T22:27:31Z

examples/babel-dynamically-entries/package.json

+    "babel-polyfill": "6.13.0",
+    "babel-preset-env": "^1.6.0",
+    "serverless-webpack": "file:../../",
+    "webpack": "^1.13.1"


As we support any webpack version now, maybe using webpack 2 in the examples would be good. What do you think?

... or at least in some examples.

👍
I'm going to update it

I've updated all examples dependencies and tested all folders.
It looks like everything is working as expected.
Also I've added yarn to examples.

- Adding yarn to examples - Fix typescript example

franciscocpg · 2017-07-25T15:10:00Z

@HyperBrain
I've added tests (file validate.test.js).

HyperBrain · 2017-07-26T11:21:38Z

I found an issue with handlers in arbitrary paths when testing with one of our bigger production projects.
I fixed it and added the commits. All my tests now worked properly.
Additionally I added lodash and used the serverless framework to retrieve the service functions which is the proposed way to do that.
This should be good to go now :)

franciscocpg · 2017-07-26T13:34:30Z

Nice @HyperBrain!
Just a note about this commit

b4a9e1c Removed yarn lock file. yarn should use package-lock.json

It looks like yarn isn't using package-lock.json right now: yarnpkg/yarn#3614
So if I ran yarn at the root of this repository, it's going to generate a yarn.lock.
According to the issue above right now we should choose to support package-lock.json (npm) OR yarn.lock (yarn).
Maybe in the future we could use yarn with package-lock.json.
This thread also may be helpful: https://stackoverflow.com/a/44904494

HyperBrain · 2017-07-26T16:16:00Z

@franciscocpg Yeah, at the moment it seems to be quite a mess with the different package lock mechanisms. Maybe we should remove the lock files completely for now, and use it as soon as the tools (npm and yarn) can work with the same one.

franciscocpg · 2017-07-26T16:28:56Z

@HyperBrain
IMO we should remove package-lock.json and add yarn.lock. Of course this means that we are "officially" migrating our node package manager to yarn (maybe we should open another issue for this discussion?).
But I don't think is a good ideia to remove lock files for now. I think we should decide if we migrate to yarn (adding yarn.lock and removing package-lock.json) or keep with npm as our official node package manager (keeping package-lock.json).

franciscocpg · 2017-07-26T16:33:37Z

For this PR maybe we should keep npm and create a new issue about migration to yarn so community could contribute with opinions.
What do you think @HyperBrain ?

HyperBrain · 2017-07-26T16:36:05Z

That sounds good to me. This PR should not invent or change things other than the problem solved ;-)
I will open a separate issue for the yarn/npm discussion.

franciscocpg mentioned this pull request Jun 2, 2017

Fixes #107 Hook on "deploy function" command #110

Closed

StevenACoffman mentioned this pull request Jun 19, 2017

Is the project still maintained? #133

Closed

sbkn added a commit to sbkn/serverless-webpack that referenced this pull request Jun 20, 2017

patched in serverless-heaven#130

2cc540b

HyperBrain mentioned this pull request Jun 30, 2017

Into the future ... #134

Closed

HyperBrain added the pr/candidate label Jun 30, 2017

HyperBrain mentioned this pull request Jul 1, 2017

Add hook for deploying individual functions #91

Closed

HyperBrain requested changes Jul 1, 2017

View reviewed changes

This was referenced Jul 2, 2017

update lambda function #98

Closed

Pack individual functions using the new packageFunction hook #60

Closed

HyperBrain added this to the 2.1.0 milestone Jul 3, 2017

HyperBrain added feature and removed pr/candidate labels Jul 3, 2017

HyperBrain mentioned this pull request Jul 3, 2017

Single function deploy does not invoke the plugin #107

Closed

franciscocpg mentioned this pull request Jul 5, 2017

Add serverless create template #137

Closed

HyperBrain mentioned this pull request Jul 7, 2017

serverless-webpack is not compatible with new function level individual packaging #120

Closed

franciscocpg force-pushed the deploy-package-function branch from 9a83c9e to a89bd92 Compare July 24, 2017 21:55

HyperBrain force-pushed the deploy-package-function branch from a89bd92 to 0a5d842 Compare July 24, 2017 22:12

franciscocpg force-pushed the deploy-package-function branch from 0a5d842 to a89bd92 Compare July 24, 2017 22:15

HyperBrain reviewed Jul 24, 2017

View reviewed changes

franciscocpg added 2 commits July 25, 2017 10:52

- Updating examples dependencies

37b469c

- Adding yarn to examples - Fix typescript example

Adding tests

8c951de

Frank Schmid added 3 commits July 26, 2017 11:19

Removed yarn lock file. yarn should use package-lock.json

b4a9e1c

Allow handlers in arbitrary paths. Use lodash and SLS functions.

1eb2e3f

Adapted unit tests

d494b1e

Frank Schmid added 2 commits July 26, 2017 13:26

Mention new entry resolution in README

886e25c

Bundle names now contain the js ending. Fixed output in examples.

9a011fc

HyperBrain approved these changes Jul 26, 2017

View reviewed changes

HyperBrain mentioned this pull request Jul 26, 2017

Path error with SLS 1.18 when package individually is set #159

Closed

HyperBrain merged commit 22a460d into serverless-heaven:master Jul 26, 2017

franciscocpg deleted the deploy-package-function branch July 26, 2017 21:02

HyperBrain mentioned this pull request Jul 27, 2017

fix for deploying individual packages. #48

Merged

Integration with deploy package function #130

Integration with deploy package function #130

Conversation

franciscocpg commented Jun 2, 2017

afhammad commented Jun 8, 2017

franciscocpg commented Jun 30, 2017

HyperBrain commented Jun 30, 2017

HyperBrain commented Jun 30, 2017

HyperBrain commented Jun 30, 2017

franciscocpg commented Jun 30, 2017 • edited Loading

HyperBrain Jul 1, 2017

Choose a reason for hiding this comment

franciscocpg Jul 3, 2017

Choose a reason for hiding this comment

HyperBrain Jul 1, 2017

Choose a reason for hiding this comment

franciscocpg Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

HyperBrain Jul 3, 2017

Choose a reason for hiding this comment

johnf commented Jul 1, 2017

HyperBrain commented Jul 3, 2017 • edited Loading

hassankhan commented Jul 3, 2017 • edited Loading

HyperBrain commented Jul 3, 2017

HyperBrain commented Jul 5, 2017 • edited Loading

franciscocpg commented Jul 5, 2017 • edited Loading

HyperBrain commented Jul 5, 2017

franciscocpg commented Jul 20, 2017

HyperBrain commented Jul 20, 2017

HyperBrain commented Jul 24, 2017

HyperBrain commented Jul 24, 2017 • edited Loading

HyperBrain commented Jul 24, 2017

franciscocpg commented Jul 24, 2017

HyperBrain commented Jul 24, 2017

franciscocpg commented Jul 24, 2017

HyperBrain commented Jul 24, 2017

HyperBrain Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

franciscocpg Jul 24, 2017

Choose a reason for hiding this comment

franciscocpg Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

franciscocpg commented Jul 25, 2017

HyperBrain commented Jul 26, 2017

franciscocpg commented Jul 26, 2017 • edited Loading

HyperBrain commented Jul 26, 2017

franciscocpg commented Jul 26, 2017

franciscocpg commented Jul 26, 2017

HyperBrain commented Jul 26, 2017

franciscocpg commented Jun 30, 2017 •

edited

Loading

franciscocpg Jul 3, 2017 •

edited

Loading

HyperBrain commented Jul 3, 2017 •

edited

Loading

hassankhan commented Jul 3, 2017 •

edited

Loading

HyperBrain commented Jul 5, 2017 •

edited

Loading

franciscocpg commented Jul 5, 2017 •

edited

Loading

HyperBrain commented Jul 24, 2017 •

edited

Loading

HyperBrain Jul 24, 2017 •

edited

Loading

franciscocpg Jul 25, 2017 •

edited

Loading

franciscocpg commented Jul 26, 2017 •

edited

Loading