Skip to content

BUG: Package wrapper files generated by ServerlessEnterprisePlugin #68

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

Merged
merged 20 commits into from
Nov 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@ language: node_js
node_js:
- "8"
- "10"
- "11"
- "12"
- "13"

env:
env:
global:
# SERVERLESS_ACCESS_KEY (`ci` token in 1password)
- secure: "IhpN2RSDv7GuyYSvz6YVaqji7RgnnmzsDe3Dn6zT3HjwEFmvZHMaHRKhkt2/jE1OaxIXESftbM0O6OsY+9qIFzPlgN7hNEYWJnj9rrQOlqJ43OJ/fzuefeMqCvU9cjolH0qe1Qs2kqYrVig/7uwtdAmkPw+NoCNt+9H8JtaD7wGCuxY5cbiK5eNVu8Yslt8oAFyi9zDGHYa1git2qzFWAxnCIsmBCZyljEEs5k53HZHGfmRaGq/5hlLN/5+gVt1vGejz769f7zhz01JPTh4GvFevcwx+8qOaHaHAuWI/4AOKQLa8L6AW6Trwencafgg50m0TCvo2CjCDXgBE8cGimxjw2vM5cDrBbm/lxPJL2S04gLCjoiXUCOcH0s870AgZsNCNKvygBLt2YBEyNEeIhU3+DLxfMns2c+G4DQcxybLj1FxLxycf7HebyU7FR1NEOtE6jKEHykCI7h8th4Swg1KaX3IOR5BjLvCpKg4psDnhpxvgraeWWQIZAXLpas3owRqGntDFcLIz6kXQAxQfIMM0AgHD9HtpSsmM2FeMkK+HMVeGkHdMA3oxFWEAPdK6mV0mL6hcU+nvgORWjO1wjQcIEaBFN9HgwXGVyrkZppa+bR0IXUYnQFJFAqirp9R/864husQS836MFhRFvGKlsmo0iUoTWuvaXLErLF9vii8="

branches:
only:
Expand All @@ -20,7 +27,6 @@ script:
- yarn --version
- yarn run check
- yarn benchmark:build
- yarn check:git-dirty
- yarn benchmark:install
- yarn test:cli
- TEST_PARALLEL=true yarn benchmark
Expand Down
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
Changes
=======

## UNRELEASED

* BUG: Hack a fix to generate wrapper files like `ServerlessEnterprisePlugin` does.
[#67](https://github.com/FormidableLabs/serverless-jetpack/pull/67)
* Infra: Add node13 to Travis CI matrix. Bump Appveyor to node10.
* Deps: Update to `serverless@^1.57.0` in all scenarios. Refactor local path plugins to use new syntax.

## 0.6.0

* README: Fix incorrect language about `foo` vs `foo/**`. See [`fast-glob` notes](https://github.com/mrmlnc/fast-glob#how-to-exclude-directory-from-reading).
Expand Down
10 changes: 10 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ test/packages/
├── complex
│   ├── npm
│   └── yarn
├── dashboard
│   ├── npm
│   └── yarn
├── huge
│   ├── npm
│   └── yarn
Expand Down Expand Up @@ -115,6 +118,13 @@ $ yarn benchmark
$ yarn benchmark:test
```

*Serverless Enterprise*: Unfortunately, these tests require a login account and special `SERVERLESS_ACCESS_KEY` environment variable. The Jetpack project has two active tokens for `localdev` and `ci`. You can enable these and our `dashboard` tests with something like:

```sh
$ SERVERLESS_ACCESS_KEY="<INSERT_HERE>" yarn benchmark
$ SERVERLESS_ACCESS_KEY="<INSERT_HERE>" yarn benchmark:test
```

## Before submitting a PR...

Before you go ahead and submit a PR, make sure that you have done the following:
Expand Down
5 changes: 3 additions & 2 deletions appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ environment:
matrix:
# Limit windows builds because they're 15 mins in serial.
# - nodejs_version: "8.10.0"
- nodejs_version: "8"
- nodejs_version: "10"
# - nodejs_version: "10"
# - nodejs_version: "11"
# - nodejs_version: "12"
# - nodejs_version: "13"

branches:
only:
Expand Down
53 changes: 51 additions & 2 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,58 @@ class Jetpack {
}
};

this.hooks = {
// The `ServerlessEnterprisePlugin` awkwardly wreaks havoc with alternative
// packaging.
//
// `serverless` special cases it and ensures it's _always_ the last plugin.
// This means that our plugin order ends up looking like:
//
// - ...
// - 'Package',
// - ...
// - 'Jetpack',
// - 'ServerlessEnterprisePlugin'
//
// Unfortunately, the plugin hooks end up like this:
//
// - `Jetpack:before:package:createDeploymentArtifacts`:
// Bundles all files, creating artifact to avoid `Package` doing it.
// - `ServerlessEnterprisePlugin:before:package:createDeploymentArtifacts`:
// Creates the `s_<NAME>.js` wrapper files.
// - `Package:package:createDeploymentArtifacts`:
// Creates any artifacts that don't already exist.
//
// This means that Jetpack can't easily get both the SFE wrappers and still
// control packaging:
//
// - If Jetpack stays with `before:package:createDeploymentArtifacts`
// it misses the `s_<NAME>.js` wrapper files.
// - _But_, if Jetpack hooks to `package:createDeploymentArtifacts` then
// `Package` will actually perform real packaging and it's too late.
//
// To get around this awkward situation, we create a hooks object that is
// delayed until the `initialize` lifecycle, then patched in last. This
// ensures that Jetpack's hooks run absolutely last for these events. This
// is still a bit hacky, but not nearly as invasive as some of the other
// approaches we considered. H/T to `@medikoo` for the strategy:
// https://github.com/FormidableLabs/serverless-jetpack/pull/68#issuecomment-556987101
const delayedHooks = {
"before:package:createDeploymentArtifacts": this.package.bind(this),
"before:package:function:package": this.package.bind(this),
"before:package:function:package": this.package.bind(this)
};

this.hooks = {
// Use delayed hooks to guarantee we are **last** to run so other things
// like the Serverless Enterprise plugin run before us.
initialize: () => {
const { hooks } = serverless.pluginManager;
Object.keys(delayedHooks).forEach((event) => {
hooks[event] = (hooks[event] || []).concat({
pluginName: this.constructor.name,
hook: delayedHooks[event]
});
});
},
Copy link

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yay! Thanks for the review and suggestion!

"jetpack:package:package": this.package.bind(this)
};
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
"mocha": "^6.1.4",
"mock-fs": "^4.9.0",
"p-queue": "^5.0.0",
"serverless": "^1.41.1",
"serverless": "^1.57.0",
"strip-ansi": "^5.2.0"
}
}
64 changes: 58 additions & 6 deletions test/benchmark.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,18 @@ const globby = require("globby");
const AdmZip = require("adm-zip");

const { TEST_SCENARIO } = process.env;
const IS_SLS_ENTERPRISE = !!process.env.SERVERLESS_ACCESS_KEY;
const { MATRIX } = require("./script");
const BASELINE_COMP_MATRIX = MATRIX.filter(({ scenario }) => scenario !== "monorepo");
const BASELINE_COMP_MATRIX = MATRIX.filter(({ scenario }) => {
if (scenario === "monorepo") {
return false;
}
// SFE-only scenarios.
if (scenario === "dashboard" && !IS_SLS_ENTERPRISE) {
return false;
}
return true;
});

// Filter known false positives.
//
Expand All @@ -33,6 +43,10 @@ const SLS_FALSE_POSITIVES_WIN_BASE = [
"node_modules/.bin/esparse",
// yarn why esvalidate -> esprima serverless#js-yaml
"node_modules/.bin/esvalidate",
// yarn why find-requires -> serverless#ncjsm
"node_modules/.bin/find-requires",
// yarn why @serverless/cli -> serverless#@serverless#cli
"node_modules/.bin/components",
// yarn why flat -> serverless#@serverless#enterprise-plugin
"node_modules/.bin/flat",
// yarn why is-ci -> serverless#update-notifier
Expand All @@ -43,6 +57,8 @@ const SLS_FALSE_POSITIVES_WIN_BASE = [
"node_modules/.bin/json-refs",
// yarn why mkdirp -> serverless
"node_modules/.bin/mkdirp",
// yarn why prettyoutput -> serverless#@serverless#cli
"node_modules/.bin/prettyoutput",
// yarn why raven -> serverless
"node_modules/.bin/raven",
// yarn why rc -> serverless
Expand Down Expand Up @@ -71,7 +87,12 @@ const SLS_FALSE_POSITIVES_WIN_BASE = [
"node_modules/.bin/which",
// yarn why yamljs -> serverless#@serverless#enterprise-plugin
"node_modules/.bin/yaml2json",
"node_modules/.bin/json2yaml"
"node_modules/.bin/json2yaml",

// Not exactly sure why, but consistently getting:
// `node_modules/bin/<NAME>.ps1` extras. Just ignore.
"node_modules/.bin/mime",
"node_modules/.bin/loose-envify"
];

// ... and the huge scenario has even more false positives
Expand Down Expand Up @@ -137,6 +158,17 @@ const SLS_FALSE_POSITIVES_WIN_HUGE = [
// a lot of `jest` dependencies are `devDependencies` when installing with
// `yarn` (although `npm` looks correct).
const SLS_FALSE_POSITIVES = {
"dashboard/yarn": new Set([
...SLS_FALSE_POSITIVES_WIN_BASE,

// Hoisted to `node_modules/.bin/mime`
"node_modules/send/node_modules/.bin/mime"
]),

"dashboard/npm": new Set([
...SLS_FALSE_POSITIVES_WIN_BASE
]),

"simple/yarn": new Set([
...SLS_FALSE_POSITIVES_WIN_BASE,

Expand Down Expand Up @@ -170,7 +202,7 @@ const SLS_FALSE_POSITIVES = {
// $ yarn why uuid -> serverless, raven
"node_modules/uuid",

// Jetpack properly excludes with `roots` (not availabel in Serverless)
// Jetpack properly excludes with `roots` (not available in Serverless)
"nodejs/node_modules/.yarn-integrity",
"nodejs/node_modules/.bin/uuid",
"nodejs/node_modules/uuid"
Expand All @@ -183,7 +215,7 @@ const SLS_FALSE_POSITIVES = {
// (`manual_test_websocket/scripts/serverless..yml`)
"node_modules/serverless-offline",

// Jetpack properly excludes with `roots` (not availabel in Serverless)
// Jetpack properly excludes with `roots` (not available in Serverless)
"nodejs/node_modules/.bin/uuid",
"nodejs/node_modules/uuid"
]),
Expand Down Expand Up @@ -237,7 +269,7 @@ const SLS_FALSE_POSITIVES = {
// $ yarn why @cnakazawa/watch -> jest#jest-cli#@jest/core#jest-haste-map#sane
"node_modules/.bin/watch",

// Hoisted to `node_moduels/.bin/loose-envify`
// Hoisted to `node_modules/.bin/loose-envify`
"node_modules/react-dom/node_modules/.bin/loose-envify",
"node_modules/react-dom/node_modules/prop-types/node_modules/.bin/loose-envify",
"node_modules/react/node_modules/.bin/loose-envify",
Expand All @@ -251,11 +283,19 @@ const SLS_FALSE_POSITIVES = {
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp#nopt"
"node_modules/abbrev",

// $ yarn why are-we-there-yet
// - "fsevents#node-pre-gyp#npmlog"
"node_modules/are-we-there-yet",

// $ yarn why console-control-strings
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp#npmlog" depends on it
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp#npmlog#gauge"
"node_modules/console-control-strings",

// $ yarn why delegates
// - "fsevents#node-pre-gyp#npmlog#are-we-there-yet"
"node_modules/delegates",

// $ yarn why detect-libc
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp" depends on it.
"node_modules/detect-libc",
Expand All @@ -264,6 +304,14 @@ const SLS_FALSE_POSITIVES = {
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp#tar"
"node_modules/fs-minipass",

// $ yarn why gauge
// - "fsevents#node-pre-gyp#npmlog"
"node_modules/gauge",

// $ yarn why has-unicode
// - "fsevents#node-pre-gyp#npmlog#gauge"
"node_modules/has-unicode",

// $ yarn why ignore-walk
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp#npm-packlist"
"node_modules/ignore-walk",
Expand Down Expand Up @@ -304,6 +352,10 @@ const SLS_FALSE_POSITIVES = {
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp"
"node_modules/npm-packlist",

// $ yarn why npmlog
// - "fsevents#node-pre-gyp"
"node_modules/npmlog",

// $ yarn why osenv
// - "jest#jest-cli#@jest/core#jest-haste-map#fsevents#node-pre-gyp#nopt"
"node_modules/osenv",
Expand Down Expand Up @@ -339,7 +391,7 @@ const keepBaselineMatch = ({ scenario, mode }) => (f) => {

// Exact match for .bin, top-level for everything else.
return f.indexOf("node_modules/.bin/") !== -1
? !matches.has(f.replace(/\.cmd$/, "")) // match unix or windows script
? !matches.has(f.replace(/\.(cmd|ps1)$/, "")) // match unix or windows script
: !matches.has(topLevel(f));
};

Expand Down
Loading