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

Allow file() variable to support TypeScript format #8071

Closed
dl748 opened this issue Aug 10, 2020 · 18 comments
Closed

Allow file() variable to support TypeScript format #8071

dl748 opened this issue Aug 10, 2020 · 18 comments

Comments

@dl748
Copy link

dl748 commented Aug 10, 2020

Use case description

Allow file() to accept more than just ".js" files to be executed. e.g. .ts and .coffee
This can require an optional module for support

serverless.json

{
  "service": "${file(./config.ts)}",
  "functions": "${file(./config.coffee)}"
}

Proposed solution

Use the interpret package to attempt to use a "require" loader (based on extension), if not, fall back to current processing.

@dl748 dl748 changed the title Allow file() to support other module files formats Allow file() to support other module file formats Aug 10, 2020
@medikoo
Copy link
Contributor

medikoo commented Aug 17, 2020

@dl748 thanks for request. What is the use case? You've just explained what you want, but not why you want it.

Also what value do you see in supporting CoffeeScript?

@dl748
Copy link
Author

dl748 commented Aug 17, 2020

Personally, I don't have value in that. But I do have value in TypeScript, where I already have WebPack, and Gulp in full TypeScript, and I'd like to add this support to serverless in the effort of having all executed files as TypeScript.

I just figured, if we were to add support for TypeScript why stop there, and we should add whatever tools the developer is comfortable with.

.e.g. Webpack, i have all my files as webpack.config.ts, and my gulp configuration as gulpfile.ts

Otherwise, I have some typescript and some javascript (untyped).

All my handlers are TypeScript, and Id like the configuration options to use TypeScript as well, instead of Javascript

@medikoo
Copy link
Contributor

medikoo commented Aug 18, 2020

Personally, I don't have value in that

I don't think we should invest time in things that are just nice to have and doesn't address any real world use case at given point (note that currently we lack resources to address real needs)

I'm going to close this now, but we're definitely open to revisit that when eventual demand will appear

@medikoo medikoo closed this as completed Aug 18, 2020
@dl748
Copy link
Author

dl748 commented Aug 18, 2020

So what about TypeScript support, which I DO want? My real world issue is that I'm forced to use javascript for my file() variables, and that I have a need for using TypeScript (.ts) files instead of javascript. I'm not sure why this was closed because I don't have a specific use for CoffeeScript, as this issue was supporting more than just that.

@medikoo medikoo changed the title Allow file() to support other module file formats Allow file() variable to support TypeScript format Aug 19, 2020
@medikoo
Copy link
Contributor

medikoo commented Aug 19, 2020

So what about TypeScript support, which I DO want?

Sorry that was not perfectly clear to me. I'll reopen this. As we support TypeScript as top file configuration file format, it definitely makes sense to support TS in variable file service scope.

PR that provides that is welcome!

@dl748
Copy link
Author

dl748 commented Aug 20, 2020

The PR that i provided does that as well, it just happens to also give support for several other files as well.

It adds support for
.ts
.tsx
.jsx
.babel.js
.babel.ts
.buble.js
.cirru
.cjsx
.co
.coffee
.coffee.md
.csv
.eg
.esm.js
.iced
.iced.md
.ini
.json5
.litcoffee
.liticed
.ls // livescript
.mjs
.node
.toml
.wisp
.xml

All in one small PR

@medikoo
Copy link
Contributor

medikoo commented Aug 21, 2020

The PR that i provided does that as well, it just happens to also give support for several other files as well.

There are problems with it.

  1. I believe there should be parity between file formats we support as top level configuration and as confiuguration extensions. Currently the only flaw is that we do not support TS file as file var scope extensions (and PR that brings that is definitely welcome)

  2. We should not make any transpilers a dependency of a Framework. Support for TypeScript was added without adding any depedency (it's users that wish to use TypeScript needs to ensure the transpiler in its environment)

@dl748
Copy link
Author

dl748 commented Aug 21, 2020

The PR that i provided does that as well, it just happens to also give support for several other files as well.

There are problems with it.

  1. I believe there should be parity between file formats we support as top level configuration and as confiuguration extensions. Currently the only flaw is that we do not support TS file as file var scope extensions (and PR that brings that is definitely welcome)
  2. We should not make any transpilers a dependency of a Framework. Support for TypeScript was added without adding any depedency (it's users that wish to use TypeScript needs to ensure the transpiler in its environment)

Note: this just provides support, the PR doesn't add ANY transpilers or preprocessors as a dependency. If a developer wants to have that support, they have to install the transpiler to give them that support similar to how your top level works. The included module is just a listing of require() register packages, and it'll "try" them if they are installed. Its VERY lightweight.

Why do you want to limit a developers choice when you don't have to?

@dl748
Copy link
Author

dl748 commented Aug 21, 2020

For example, if you want to use serverless.ts you have to install a package like ts-node.... similar to my addition to file(), its just that my PR allows for A LOT more extensions than just .ts, and gives developers a lot more choices for their build system. The package that i'm using "interpret" has ZERO dependencies, its just a well maintained list of registered modules by file extensions and will use them if they are installed, nothing more. In fact, I could see this being modified to top level configuration as well.

@medikoo
Copy link
Contributor

medikoo commented Aug 24, 2020

the PR doesn't add ANY transpilers or preprocessors as a dependency

Indeed, it just adds dependency of interpret, which on its own doesn't bring any transpilers.

Why do you want to limit a developers choice when you don't have to?

As I mentioned due to limited resources we're focused on addressing only real world (and not "nice to have") use cases. As we agreed, there's only one real use case to address here (support TS files via variable extensions) and we're open for PR on that.

@dl748
Copy link
Author

dl748 commented Sep 14, 2020

Closing, I've found my workaround for this feature.

@dl748 dl748 closed this as completed Sep 14, 2020
@G-Rath
Copy link
Contributor

G-Rath commented Feb 14, 2021

@medikoo I'm interested in this, and might try to land a PR for it in the near future - just want to confirm first that this is still true:

Currently the only flaw is that we do not support TS file as file var scope extensions (and PR that brings that is definitely welcome)

It should in theory be a pretty straightforward patch as it should be the same logic as what's used for serverless.ts, but don't want to do the work if there's no interest in landing it :)

@medikoo
Copy link
Contributor

medikoo commented Feb 15, 2021

@G-Rath I think as we support TS files being top level, it makes sense to supporting then in file: variables. Still any work on that should be done with consideration for following:

  1. Currently we're discussing moving TS support to external repository -> Support tsconfig-paths and specify tsconfig*.json typescript#28 (comment)
  2. Currently we're rewriting variables parser and resolver from scratch (it's in context of Seclude CLI params and service config resolution from the core #8364, and PR with new resolver will posted within few days).

Having that it's probably worth to wait a bit, until above matters settle, and implementing this in current variables resolver context doesn't make much sense, as it's about to be ditched completely.

@G-Rath
Copy link
Contributor

G-Rath commented Feb 15, 2021

Fair enough - I don't mind waiting if there's something actually in the works that's expected to land in a few weeks.

When it does land, feel free to ping me - I'd be happy to get this feature landed asap as I'm in need of it :)

@medikoo
Copy link
Contributor

medikoo commented Feb 15, 2021

Great thanks @G-Rath for raising an interest! I'll definitely update this issue, once we will settle on recommend approach on solving this issue

@pushred
Copy link

pushred commented Sep 6, 2023

@medikoo This still seems to be an issue following the #8364 changes which I understood were to be a new starting point for whatever solution addresses this particular need. But is a new issue warranted now? I'm not seeing that #28 would address this and also appears stuck.

When I reference a TS file with the file() variable syntax I get an error that doesn't quite make sense:

Cannot resolve serverless.ts: Variables resolution errored with:
  - Cannot resolve variable at "example.path": Value not found at "env" source

Environment: darwin, node 18.14.2, framework 3.27.0 (local), plugin 6.2.3, SDK 4.3.2

@medikoo
Copy link
Contributor

medikoo commented Sep 6, 2023

@pushred in my eyes best way to support it is via introducing extensions as proposed here: #9311

Yet, I'm no longer with Serverless Inc., and I'm not familiar with current roadmap and priorities, I also don't have any rights to accept and release any further updates here (as far as I know it's @Danwakeem that takes care of Framework maintenance now)

@pushred
Copy link

pushred commented Sep 6, 2023

@medikoo ah thank you for such a quick update on where this stands! #9311 does look sensible, will watch that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants