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

add serverless.ts support #7755

Merged
merged 11 commits into from
Jun 1, 2020
2 changes: 2 additions & 0 deletions lib/classes/Utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ class Utils {
servicePath = process.cwd();
} else if (fileExistsSync(path.join(process.cwd(), 'serverless.js'))) {
servicePath = process.cwd();
} else if (fileExistsSync(path.join(process.cwd(), 'serverless.ts'))) {
servicePath = process.cwd();
}

return servicePath;
Expand Down
12 changes: 12 additions & 0 deletions lib/classes/Utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,18 @@ describe('Utils', () => {
expect(servicePath).to.not.equal(null);
});

it('should detect if the CWD is a service directory when using Serverless .ts files', () => {
const tmpDirPath = getTmpDirPath();
const tmpFilePath = path.join(tmpDirPath, 'serverless.ts');

serverless.utils.writeFileSync(tmpFilePath, 'foo');
process.chdir(tmpDirPath);

const servicePath = serverless.utils.findServicePath();

expect(servicePath).to.not.equal(null);
});

it('should detect if the CWD is not a service directory', () => {
// just use the root of the tmpdir because findServicePath will
// also check parent directories (and may find matching tmp dirs
Expand Down
24 changes: 17 additions & 7 deletions lib/utils/getServerlessConfigFile.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ const getConfigFilePath = (servicePath, options = {}) => {
const ymlPath = path.join(servicePath, 'serverless.yml');
const yamlPath = path.join(servicePath, 'serverless.yaml');
const jsPath = path.join(servicePath, 'serverless.js');
const tsPath = path.join(servicePath, 'serverless.ts');

return BbPromise.props({
json: fileExists(jsonPath),
yml: fileExists(ymlPath),
yaml: fileExists(yamlPath),
js: fileExists(jsPath),
ts: fileExists(tsPath),
}).then(exists => {
if (exists.yaml) {
return yamlPath;
Expand All @@ -33,6 +35,8 @@ const getConfigFilePath = (servicePath, options = {}) => {
return jsonPath;
} else if (exists.js) {
return jsPath;
} else if (exists.ts) {
return tsPath;
}

return null;
Expand All @@ -46,28 +50,34 @@ const getServerlessConfigFilePath = serverless => {
);
};

const handleJsConfigFile = jsConfigFile =>
const handleJsOrTsConfigFile = (configFile, isTs) =>
BbPromise.try(() => {
// use require to load serverless.js
if (isTs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not introduce isTs argument, but simply detect that by configFile.endsWith('.ts')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

// eslint-disable-next-line global-require
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this comment (we do not have this rule turned on)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

require('ts-node').register();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not add it as dependency. I think TS users should just ensure that both typescript and ts-node are installed in their setup.

Also we need to ensure it'll work with standalone build, which makes it a bit more tricky (as it doesn't have natural access to service or global npm dependencies)

I imagine reliable logic of acquisition of ts-node as follows:

const resolveModulePath = require('ncjsm/resolve');
const spawn = require('child-process-ext/spawn')
...

const tsNode = () => {
	// 1. Resolve as serverless peer dependency
	return resolveModulePath(__dirname, "ts-node")
		.catch((error) => {
			if (error.code !== "MODULE_NOT_FOUND") throw error;
			// 2. Resolve as service dependency
			return resolveModulePath(path.dirname(configFile), "ts-node");
		})
		.catch((error) => {
			if (error.code !== "MODULE_NOT_FOUND") throw error;
			// 2. Resolve as global installation
			return spawn("npm", ["root", "-g"]).then(
				({ stdoutBuffer }) => {
					return resolveModulePath("/", `${String(stdoutBuffer).trim()}/ts-node`).catch(
						(error) => {
							if (error.code !== "MODULE_NOT_FOUND") throw error;
							return null;
						}
					);
				},
				(error) => null
			);
		})
		.then((tsNodePath) => {
			if (!tsNodePath) {
				throw new ServerlessError(
					"Ensure 'ts-node' dependency for working with TypeScript configuration files"
				);
			}
			return require(tsNodePath);
		});
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this to remove ts-node and resolve it here - some weird stuff I had to do to get it to resolve symlinks (because it returns an object instead of filepath), but works for all 3 cases. Let me know if you like this approach, or if you want to include ts-node as a dep.

}
// use require to load serverless config file
// eslint-disable-next-line global-require
const configExport = require(jsConfigFile);
const configExport = require(configFile);
// In case of a promise result, first resolve it.
return configExport;
}).then(config => {
if (_.isPlainObject(config)) {
return config;
}
throw new Error('serverless.js must export plain object');
throw new Error(`serverless.${isTs ? 'ts' : 'js'} must export plain object`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's resolve config filename via path.basename instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

});

const getServerlessConfigFile = _.memoize(
serverless =>
getServerlessConfigFilePath(serverless).then(configFilePath => {
if (configFilePath !== null) {
const isJSConfigFile = _.last(_.split(configFilePath, '.')) === 'js';
const fileExtension = _.last(_.split(configFilePath, '.'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's in old code like that, but maybe refactor it to use path.ext. This lodash usage is highly unjustified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified to use path.extname and changed the if check to include the . on .ts and .js

const isJSConfigFile = fileExtension === 'js';
const isTSConfigFile = fileExtension === 'ts';

if (isJSConfigFile) {
return handleJsConfigFile(configFilePath);
if (isJSConfigFile || isTSConfigFile) {
return handleJsOrTsConfigFile(configFilePath, isTSConfigFile);
}

return readFile(configFilePath).then(result => result || {});
Expand Down
43 changes: 43 additions & 0 deletions lib/utils/getServerlessConfigFile.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,49 @@ describe('#getServerlessConfigFile()', () => {
).to.be.rejectedWith('serverless.js must export plain object');
});

it('should return the file content if a serverless.ts file found', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For tests we should use runServerless util, as documented here: https://github.com/serverless/serverless/blob/master/tests/README.md#unit-tests

It's the only reliable way to confirm that it works. We may prepare some basic fixture that involves serverless.ts config (but with no real TS internals, to not impose TS dependency on test engine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a fixture and added a test case. Let me know if you think there's more that I should be testing

const serverlessFilePath = path.join(tmpDirPath, 'serverless.ts');
writeFileSync(serverlessFilePath, 'module.exports = {"service": "my-ts-service"};');

return expect(
getServerlessConfigFile({
processedInput: { options: {} },
config: { servicePath: tmpDirPath },
})
).to.be.fulfilled.then(result => {
expect(result).to.deep.equal({ service: 'my-ts-service' });
});
});

it('should return the resolved value if a promise-using serverless.ts file found', () => {
const serverlessFilePath = path.join(tmpDirPath, 'serverless.ts');
writeFileSync(
serverlessFilePath,
'module.exports = new Promise(resolve => { resolve({"service": "my-ts-service"}); });'
);

return expect(
getServerlessConfigFile({
processedInput: { options: {} },
config: { servicePath: tmpDirPath },
})
).to.be.fulfilled.then(result => {
expect(result).to.deep.equal({ service: 'my-ts-service' });
});
});

it('should throw an error, if serverless.ts export not a plain object', () => {
const serverlessFilePath = path.join(tmpDirPath, 'serverless.ts');
writeFileSync(serverlessFilePath, 'module.exports = function config() {};');

return expect(
getServerlessConfigFile({
processedInput: { options: {} },
config: { servicePath: tmpDirPath },
})
).to.be.rejectedWith('serverless.ts must export plain object');
});

it('should look in the current working directory if servicePath is undefined', () => {
const serverlessFilePath = path.join(tmpDirPath, 'serverless.yml');

Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@
"semver-regex": "^2.0.0",
"stream-promise": "^3.2.0",
"tabtab": "^3.0.2",
"ts-node": "^8.10.1",
"untildify": "^3.0.3",
"update-notifier": "^2.5.0",
"uuid": "^8.0.0",
Expand Down