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 support for webpack configuration in TypeScript format #129

Merged
merged 2 commits into from
Jul 20, 2017
Merged

Add support for webpack configuration in TypeScript format #129

merged 2 commits into from
Jul 20, 2017

Conversation

jussikinnula
Copy link
Contributor

To add support for webpack configuration in TypeScript format (e.g. webpack.config.ts), there's an existing package ts-node which just does the magic.

Webpack uses this method also internally, if specifying configuration file to webpack by using webpack --config webpack.config.ts. But since serverless-webpack does require the configuration by using a "require", ts-node needs to be required to support on-the-fly .ts -file requires.

@HyperBrain HyperBrain mentioned this pull request Jun 30, 2017
@hassankhan
Copy link
Contributor

Shouldn't we detect whether a TypeScript format configuration is being used before loading ts-node?

@HyperBrain
Copy link
Member

I agree with @hassankhan . Unconditionally requiring ts-node and triggering all its side-effects maybe unwanted if you don't use TypeScript at all.
What about adding a command line switch --ts to conditionally enable TypeScript support on demand and initialize (require) it only then?

@hassankhan
Copy link
Contributor

I was thinking of something more lo-fi like checking the file extension, either way sounds good 👍

@jussikinnula
Copy link
Contributor Author

I originally had the require('ts-node/register'); conditionally, just before starting webpack compilation. So I'll update the PR next week when I'm back at work :-) ...

I wouldn't worry about any side effects, since ts-node actually does hook into require() and .ts -files. But of course it's a consideration always to add any extra dependencies.

@HyperBrain HyperBrain added this to the 2.1.0 milestone Jul 3, 2017
@HyperBrain
Copy link
Member

@jussikinnula Any update here?

@jussikinnula
Copy link
Contributor Author

Updated version of the patch is now on top of latest upstream master!

I tested with my Angular starter project, and works (e.g. npm run sls:local):
https://github.com/jussikinnula/angular-serverless-typescript

@@ -15,6 +15,15 @@ class ServerlessWebpack {
this.serverless = serverless;
this.options = options;

if (
this.serverless.service
Copy link
Contributor

@hassankhan hassankhan Jul 12, 2017

Choose a reason for hiding this comment

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

Could these perhaps use Lodash's has() instead? Something like:

if (
_.has(this.serverless.service, 'custom.webpack') 
&& _.endsWith(this.serverless.service.custom.webpack, '.ts')
) {
...
}

Copy link
Member

@HyperBrain HyperBrain Jul 12, 2017

Choose a reason for hiding this comment

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

.. and lodash's _.endsWith() 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the comment 😉

@hassankhan
Copy link
Contributor

Aside from the one thing mentioned, this looks good to go! 👍

@jussikinnula
Copy link
Contributor Author

I didn't want to add any extra dependencies besides ts-node, and also I tried to follow the coding style used elsewhere. If you want I can add lodash and use it's patterns, but I guess it would be a separate thing since similar if -clauses exist in various places under lib/.

@HyperBrain
Copy link
Member

That's a valid point. We can do a "lodash"ification as a completely separate branch/PR and switch the code to use it everywhere, not only here. I will create a task for that, so that we get the whole codebase a bit leaner.
So this one is good to go.

@HyperBrain HyperBrain merged commit f4988de into serverless-heaven:master Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants