Remove BlueBird form zipService

[tstackhouse]

Based on discussion in #9542, adding a PR to refactor out blubbird from zipService

Note: I get a linting failure, but there's no other way to use async/await.

[medikoo]

@tstackhouse Great thanks for this PR. Still it touches a lot of sensitive parts, and we've paved a more cautious migration plan from Bluebird outlined here: #7171

Unfortunately we have a bad history of regressions when merging PR's like that, so we would prefer to not risk that again, and stick to plan outlined above.

In #9542 (review) I've suggested that we simply refactor zipService method. So just what's highlighted here:

serverless/lib/plugins/package/lib/zipService.js

Lines 59 to 104 in 8ac249b

	zipFiles(files, zipFileName, prefix) {
	if (files.length === 0) {
	const error = new ServerlessError('No files to package', 'NO_FILES_TO_PACKAGE');
	return BbPromise.reject(error);
	}
	const zip = archiver.create('zip');
	// Create artifact in temp path and move it to the package path (if any) later
	const artifactFilePath = path.join(this.serverless.serviceDir, '.serverless', zipFileName);
	this.serverless.utils.writeFileDir(artifactFilePath);
	const output = fs.createWriteStream(artifactFilePath);
	return new BbPromise((resolve, reject) => {
	output.on('close', () => resolve(artifactFilePath));
	output.on('error', (err) => reject(err));
	zip.on('error', (err) => reject(err));
	output.on('open', () => {
	zip.pipe(output);
	// normalize both maps to avoid problems with e.g. Path Separators in different shells
	const normalizedFiles = _.uniq(files.map((file) => path.normalize(file)));
	return BbPromise.all(normalizedFiles.map(this.getFileContentAndStat.bind(this)))
	.then((contents) => {
	contents
	.sort((content1, content2) => content1.filePath.localeCompare(content2.filePath))
	.forEach((file) => {
	const name = file.filePath.slice(prefix ? `${prefix}${path.sep}`.length : 0);
	// Ensure file is executable if it is locally executable or
	// we force it to be executable if platform is windows
	const mode = file.stat.mode & 0o100 || process.platform === 'win32' ? 0o755 : 0o644;
	zip.append(file.data, {
	name,
	mode,
	date: new Date(0), // necessary to get the same hash when zipping the same content
	});
	});
	zip.finalize();
	})
	.catch(reject);
	});
	});
	},

Can you update PR so it covers just that part of code?

Great thanks, and sorry if I wasn't perfectly clear in a comment

[tstackhouse]

Reading through your comments on #7171, it sounds like this is something that's going to be 2.0 exclusive? The code I had added in #9542 was designed to be compatible with 1.x, I'm assuming that that would explain why I'm getting that linting failure on async, as in 1.x's package.json the eslint config is extending @serverless/eslint-config/node/6?

[medikoo]

The code I had added in #9542 was designed to be compatible with 1.x,

master branch is for 2.x release chain.

For v1 we have v1 branch, but it's no longer maintained release (EOL), so we do not accept improvements there (unless it's quite critical security issue).

Is there any specific reason you were trying to make it v1 compatible?

[tstackhouse]

My PRs are both against that v1 branch, I didn't realize v1 was already EOL, in my project, we're using Nx and Nx Serverless, which until recently, did not support 2.x, support for which is only in beta right now, and we've ran into a wall with our CI pipeline, as we are packaging 60+ lambdas at 30+ MB each, which is running into that issue. Without going into every detail, there is a defect in Nx Serverless (flowaccount/nx-plugins#82) that I filed and have been trying to help solve, related to not packaging lambdas individually, which would sidestep the memory issue. Our build process is also not as optimized as it could be and the generated packages contain nearly identical code, I've done some optimizing with include/exclude, but that doesn't fully solve the issue, as each package still gets the pruned node_modules for the entire api, and inlining those makes webpack run put of memory.

I'm clearly rambling at this point, but the most effective solution, at least to stop the bleeding for my team, was to fork v1, make that change, and offer it upstream as a patch. This has been something I've been working on for a couple of weeks at this point, in the meantime I'm looking at how to get my project closer to the edge, using the most up to date versions of things, which is important in general for maintainability.

[medikoo]

@tstackhouse I've just noticed it's against v1 branch (originally I blindly assumed it's against v2).

Feel free to fork v1 for your needs, if upgrading to v2 is for some reason problematic (note that v2 had pretty minor changes, biggest breaking change was that we dropped support for Node.js v6 and v8, which were EOL at that point, so I'm not sure why upgrading to v2 raises a big issue).

Otherwise we're happy to accept discussed changes against master.

[tstackhouse]

I don't think it's problematic, so much as it's not (at the time) supported by the tooling we were using, after digging into that plugin, it tends to have a lot of hooks into the internals of serverless, starting with invoking it programmatically rather than via simple cli invocation, as well as remapping some of the internals during the different phases, so as to control the output paths while working in a monorepo framework.

I'd be happy to submit some patches against master, as long as I can find the time. In the meantime I'll make use of my v1 fork until I can migrate to the newest nx-serverless (once it leaves beta) and v2 along with it.

[medikoo]

Thanks for clarifications. I'm going to close this PR, as we do not accept improvements in v1 branch