Could not include required dependency warning

[joeduffy]

I receive the warning

info: Could not include required dependency '@slack/client' in '/Users/joeduffy/temp/post-sqs-slack'.

for the below lambda. I can't figure out why it's missing it, since it definitely does exist. Is there a bug in the way we regex match scoped packages, because they exist inside of subdirectories?

import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
import * as serverless from "@pulumi/aws-serverless";

let config = new pulumi.Config(pulumi.getProject());
let queue = aws.sqs.Queue.get("my-queue", config.require("queueName"));
let slackChannel = config.get("slackChannel") || "#general";
let slackToken = config.require("slackToken");
serverless.queue.subscribe("my-slack-poster", queue, async (e: serverless.queue.QueueEvent) => {
    const slack = await import("@slack/client");
    const client = new slack.WebClient(slackToken);
    for (let rec of e.Records) {
        await client.chat.postMessage({
            channel: slackChannel,
            text: `*SQS message ${rec.messageId}* in region ${rec.awsRegion}:\n`+
                `${rec.body}\n`+
                `(with :love_letter: from Pulumi)`,
            as_user: true,
        });
    }
});

[joeduffy]

I suspect the culprit it

pulumi-aws/overlays/nodejs/serverless/function.ts

Lines 315 to 321 in 7786683

	// Note: `read-package-tree` returns incorrect `.name` properties for packages in an orgnaization - like
	// `@types/express` or `@protobufjs/path`. Compute the correct name from the `path` property instead.
	// Match any name that ends with something that looks like `@foo/bar`.
	const match = /\/\@[^\/]*\/[^\/]*$/.exec(child.path);
	if (match) {
	childName = match[0];
	}

but I really don't follow what that code is trying to do.

Attempting a fix now, since this is blocking the demo I had wanted to do, ... please shout if anybody sees or knows anything about the underlying issue here.

[joeduffy]

Yep ☹️

Notice the leading slash in /\/\@[^\/]*\/[^\/]*$/.exec(child.path). This turns names like @protobufjs/fetch into /@protobufjs/fetch, which of course will not match the desired package name.

Simple one-line fix, which I have locally, and will push. We need tests.

[lukehoban]

Argh: #230 (comment)

The attempt to address that feedback led to this incorrect leading slash in the regexp. I believe it works for nested dependencies in organizations, but not for top level dependencies in organizations.

We need tests.

💯

[joeduffy]

Ahhhhh 😑

Turns out, I am still hitting some problems. At runtime:

 2018-07-04T10:23:13.445-07:00[mySlackPoster-queue-subscripti] 2018-07-04T17:23:13.445Z	75f625b5-1fed-5b14-8c16-76021b433d29	{"errorMessage":"Cannot find module 'loglevel'","errorType":"Error","stackTrace":["Function.Module._resolveFilename (module.js:547:15)","Function.Module._load (module.js:474:25)","Module.require (module.js:596:17)","require (internal/module.js:11:18)","Object.<anonymous> (/var/task/node_modules/@slack/client/dist/logger.js:3:13)","Module._compile (module.js:652:30)","Object.Module._extensions..js (module.js:663:10)","Module.load (module.js:565:32)","tryModuleLoad (module.js:505:12)","Function.Module._load (module.js:497:3)"]}

Super strange because it is now picking up @slack/client and so no idea why it's missing part of the directory.

[joeduffy]

Hmm, well, the reason here is that @slack/client itself depends on the loglevel module, which has been installed at the top level node_modules/ directory. So, we have

node_modules/
    @slack/
        client/
            node_modules/
                <lots of stuff>
    loglevel/

It appears we happily zip up the @slack/... directory, but miss loglevel, and based on our current logic for doing this, this would seem to be "by design"?

Also, unless I'm missing something, I don't have a way of controlling what gets included in the functions created by the aws-serverless package, like I do cloud-aws?

Ultimately, I suspect #257 is our way out here, but I seem to be somewhat in a pinch at the moment ... I can try manually hacking some of my local packages.

[lukehoban]

It appears we happily zip up the @slack/... directory, but miss loglevel, and based on our current logic for doing this, this would seem to be "by design"?

I can't reproduce this. When I reference @slack/client, I see loglevel picked up and included in exactly the expected place in the node_modules tree. The existing logic handles this case of dependencies which end up being placed higher in the tree by npm/yarn.

Can you share the ZIP for you Lambda?

[lukehoban]

Also, unless I'm missing something, I don't have a way of controlling what gets included in the functions created by the aws-serverless package, like I do cloud-aws?

Ultimately, I suspect #257 is our way out here, but I seem to be somewhat in a pinch at the moment ... I can try manually hacking some of my local packages.

Both true - #257 should remove the need for any manual overrides. There may still be the need for additional user blacklisting (for performance, not correctness) which we'll need to expose config for at various layers.

[lukehoban]

I can't reproduce this. When I reference @slack/client, I see loglevel picked up and included in exactly the expected place in the node_modules tree. The existing logic handles this case of dependencies which end up being placed higher in the tree by npm/yarn.

Indeed - it looks like this does reproduce in your branch for #261, but does not reproduce when the one character fix (to remove the leading /) is made instead. I'll need to look into why that is. Currently testing a more targeted fix in #262 (including a test case for @slack/client).

[lukehoban]

Fixed with #262.