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
add serverless.ts support #7755
Conversation
yesterday I added AWS types for the combination of these 2 things could make it very easy to use Serverless with AWS and get type safety/auto completion. |
Codecov Report
@@ Coverage Diff @@
## master #7755 +/- ##
==========================================
+ Coverage 88.00% 88.18% +0.17%
==========================================
Files 245 245
Lines 9254 9293 +39
==========================================
+ Hits 8144 8195 +51
+ Misses 1110 1098 -12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryan-hunter great thanks for giving that a spin! It's definitely a worthwhile improvement for all TypeScript developers.
We'll be happy to take this in, still please see my suggestions
lib/utils/getServerlessConfigFile.js
Outdated
BbPromise.try(() => { | ||
// use require to load serverless.js | ||
if (isTs) { |
There was a problem hiding this comment.
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')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
lib/utils/getServerlessConfigFile.js
Outdated
// 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`); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
lib/utils/getServerlessConfigFile.js
Outdated
BbPromise.try(() => { | ||
// use require to load serverless.js | ||
if (isTs) { | ||
// eslint-disable-next-line global-require |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
lib/utils/getServerlessConfigFile.js
Outdated
// use require to load serverless.js | ||
if (isTs) { | ||
// eslint-disable-next-line global-require | ||
require('ts-node').register(); |
There was a problem hiding this comment.
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);
});
};
There was a problem hiding this comment.
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.
package.json
Outdated
@@ -171,6 +171,7 @@ | |||
"sinon-chai": "^3.5.0", | |||
"standard-version": "^8.0.0", | |||
"strip-ansi": "^5.2.0", | |||
"ts-node": "^8.10.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added as devDependency for test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add it.
In previous comment I proposed to use fixture and when testing in fixture we may create a dummy node_modules/ts-node.js
in a fixture folder, which should be a module that exposes a register
method. Through that we may confirm it doesn't crash and that register
is invoked
@medikoo I have removed I have updated code and added tests. Let me know what you think :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bryan-hunter for update! Please see my remarks
lib/utils/getServerlessConfigFile.js
Outdated
throw error; | ||
} | ||
// if there is a symlink, we need to get 'realPath' from the return object | ||
const pathString = typeof result === 'string' ? result : result.realPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all cases object with realPath
property would be returned (sorry I forgot to reflect that in my proposal).
So technically we should just return result.realPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
lib/utils/getServerlessConfigFile.js
Outdated
if (!result) { | ||
const error = new Error(`Cannot find module '${args[1]}'`); | ||
error.code = 'MODULE_NOT_FOUND'; | ||
throw error; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveModulePath
when module is not found will naturally crash with such error, so this seems as dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
lib/utils/getServerlessConfigFile.js
Outdated
} | ||
// if there is a symlink, we need to get 'realPath' from the return object | ||
const pathString = typeof result === 'string' ? result : result.realPath; | ||
return require(pathString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should require
here.
At this point we know that ts-node
module exists at path, and that should be a final path to be used. If for some reason require of existing ts-node
crashes, it should be exposed, and not that we should try other locations.
It'll also reflect how resolution works in Node.js (other locations are not inspected if existing module crashes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it to after the chain of catch
es once module is resolved, so won't error on require and be caught erroneously
lib/utils/getServerlessConfigFile.js
Outdated
const resolveAsServiceDependency = () => requireOrThrow(serviceDir, 'ts-node'); | ||
const resolveAsGlobalInstallation = () => | ||
spawn('npm', ['root', '-g']).then(({ stdoutBuffer }) => { | ||
return requireOrThrow('/', `${String(stdoutBuffer).trim()}/ts-node`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as we have an abosolute path, it'll be good to just use require.resolve()
(in previous example I used ncjsm resolve, but in this case it's not justified)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified to use require.resolve
lib/utils/getServerlessConfigFile.js
Outdated
}); | ||
|
||
const getServerlessConfigFile = _.memoize( | ||
serverless => | ||
getServerlessConfigFilePath(serverless).then(configFilePath => { | ||
if (configFilePath !== null) { | ||
const isJSConfigFile = _.last(_.split(configFilePath, '.')) === 'js'; | ||
const fileExtension = _.last(_.split(configFilePath, '.')); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -146,6 +148,81 @@ describe('#getServerlessConfigFile()', () => { | |||
).to.be.rejectedWith('serverless.js must export plain object'); | |||
}); | |||
|
|||
it('should return the file content if a serverless.ts file found', () => { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
package.json
Outdated
@@ -171,6 +171,7 @@ | |||
"sinon-chai": "^3.5.0", | |||
"standard-version": "^8.0.0", | |||
"strip-ansi": "^5.2.0", | |||
"ts-node": "^8.10.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add it.
In previous comment I proposed to use fixture and when testing in fixture we may create a dummy node_modules/ts-node.js
in a fixture folder, which should be a module that exposes a register
method. Through that we may confirm it doesn't crash and that register
is invoked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryan-hunter looks great! I've posted just few last minor suggestions and we should be good to go
lib/utils/getServerlessConfigFile.js
Outdated
const resolveModuleRealPath = (...args) => | ||
resolveModulePath(...args).then(({ realPath }) => realPath); | ||
|
||
const createOnError = cb => error => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename it to ifNotFoundContinueWith
(at least createOnError
doesn't reflect what this util does)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
const throwTsNodeError = () => { | ||
throw new ServerlessError( | ||
'Ensure "ts-node" dependency when working with TypeScript configuration files' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better processing (e.g. in tests), it'll be nice to add code to this error (it's accepted as second argument), e.g. 'TS_NODE_NOT_FOUND'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done and used in test :)
@@ -0,0 +1,3 @@ | |||
'use strict'; | |||
|
|||
module.exports.register = () => null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of adding this conditionally.
So first test without it (and confirm that 'TS_NODE_NOT_FOUND'
error was thrown), then create this file in test, and confirm it resolves.
Note that fixures.cleanup
will remove created file from fixture, so you don't need to worry about cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. I did have to pass in "extraPaths" option to fixture.cleanup because it only tries to cleanup .serverless
, but was easy enough and seems to work nicely.
Also had to pass in shouldStubSpawn: true
so wouldn't resolve my global npm ts-node... was nice to already have that option available - nice job on making everything easy to use though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you @bryan-hunter ! Thanks for good words on test utils, it's nice to hear you quickly managed to solve approached issues.
@medikoo do you know when it can be released? |
@bryan-hunter tomorrow morning CET time |
This would allow to replace a current serverless.yml for a serverless.ts? |
@DavidEspinosa42 this would be one example:
be sure to install @types/serverless (some of the types may be slightly incorrect there, but can easily make a PR to fix them) if you want to use Serverless types out of the box - otherwise you can use your own typings :) |
Thanks Bryan!! |
how to use the custom variables in the ts version like, how would the below
|
@bharat-tiwari You could use something like yargs: import * as yargs from 'yargs'; custom: { |
I still can figure it out how to do this:
|
@mospina have you tried with quotes? |
Closes: #6335
Very simple PR to handle
serverless.ts
files.Try to find
ts-node
module when.ts
file is detected and throw an error that you need to provide this dependency to use serverless.ts files.