Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

feat: add support for TypeScript to NFT #1515

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Conversation

eduardoboucas
Copy link
Member

@eduardoboucas eduardoboucas commented Aug 1, 2023

Summary

Currently, TypeScript files are only supported when the bundler is set to esbuild. This PR adds support for TypeScript when using NFT as the bundler.

With that, we also change the upcoming functions API v2 to use NFT by default, instead of esbuild (2779f30).

@eduardoboucas eduardoboucas requested a review from a team as a code owner August 1, 2023 11:04
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

⏱ Benchmark results

Comparing with f345c7c

largeDepsEsbuild: 2.7s

⬇️ 25.29% decrease vs. f345c7c

^                                                                           3.6s                          
│                                           3.4s                            ┌──┐            3.4s          
│                           3.1s            ┌──┐                            |  |            ┌──┐          
│                           ┌──┐            |  |                            |  |    3.1s    |  |          
│                           |  |            |  |                            |  |    ┌──┐    |  |          
│ ──2.7s────────────────────┼──┼────────────┼──┼────────────────────────────┼──┼────┼──┼────┼──┼────2.7s──
│   ┌──┐    2.5s    2.4s    |  |    2.4s    |  |    2.4s    2.5s    2.4s    |  |    |  |    |  |    ┌──┐  
│   |  |    ┌──┐    ┌──┐    |  |    ┌──┐    |  |    ┌──┐    ┌──┐    ┌──┐    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

largeDepsNft: 8.7s

⬇️ 28.84% decrease vs. f345c7c

^                                                                          11.9s                          
│                                           11s                             ┌──┐           11.2s          
│                                           ┌──┐                            |  |            ┌──┐          
│                                           |  |                            |  |            |  |          
│                           9.8s            |  |                            |  |            |  |          
│                           ┌──┐            |  |                            |  |    9.1s    |  |    8.7s  
│ ──8.5s─────8s─────────────┼──┼────────────┼──┼────────────────────────────┼──┼────┌──┐────┼──┼────┌──┐──
│   ┌──┐    ┌──┐    7.6s    |  |            |  |    7.6s    7.8s    7.5s    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    ┌──┐    |  |    7.4s    |  |    ┌──┐    ┌──┐    ┌──┐    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    ┌──┐    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

largeDepsZisi: 16.6s

⬇️ 29.44% decrease vs. f345c7c

^                                                                          22.4s                          
│                                          20.7s                            ┌──┐           21.5s          
│                          19.7s            ┌──┐                            |  |            ┌──┐          
│                           ┌──┐            |  |                            |  |   18.5s    |  |          
│                           |  |            |  |                            |  |    ┌──┐    |  |          
│ ─16.5s────────────────────┼──┼────────────┼──┼────────────────────────────┼──┼────┼──┼────┼──┼───16.6s──
│   ┌──┐   15.9s   15.1s    |  |   15.1s    |  |                   15.3s    |  |    |  |    |  |    ┌──┐  
│   |  |    ┌──┐    ┌──┐    |  |    ┌──┐    |  |   14.9s   14.9s    ┌──┐    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    ┌──┐    ┌──┐    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@@ -109,6 +118,19 @@ const traceFilesAndTranspile = async function ({
ignore: getIgnoreFunction(config),
readFile: async (path: string) => {
try {
if (extname(path) === '.ts') {
Copy link
Contributor

Choose a reason for hiding this comment

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

whats about .mts or .ctsif working in ESM or commonjs scopes?

Maybe some \.(c|m)?ts$ regex?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏻 I'll do this as a follow-up.

@@ -109,6 +118,19 @@ const traceFilesAndTranspile = async function ({
ignore: getIgnoreFunction(config),
readFile: async (path: string) => {
try {
if (extname(path) === '.ts') {
const transpiledSource = await transpile({ config, name, path })
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume config is the tsconfig?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, config is the config block for the function that comes from both netlify.toml and the in-source configuration.

return {
aliases: tsAliases,
mainFile: getPathWithExtension(mainFile, '.js'),
moduleFormat: MODULE_FORMAT.ESM,
Copy link
Contributor

Choose a reason for hiding this comment

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

is only esm supported? or shouldn't this be read from the tsconfig settings?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're making the v2 API ESM only, so I think hardcoding that setting here is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although, it might still be useful for people wanting to use NFT with TypeScript on v1 functions, for some reason. I'll create an issue to do this as a follow-up.

@@ -34,6 +36,8 @@ const bundle: BundleFunction = async ({
includedFilesBasePath || basePath,
)
const {
aliases,
mainFile: normalizedMainFile,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you elaborate on what normalizedMainFile is? Is it a normalized path to the bundle entry file?

Copy link
Member Author

Choose a reason for hiding this comment

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

The mainFile property refers to both the path of a function's entry file, as found in a Netlify project, and to the path of the entry file inside the generated bundle.

Most times this file name is the same, because we find function.js and generate function.js. But when transpiling, we need to rewrite function.ts as function.js, and that's what normalizedMainFile refers to.

Although we use it in a bunch of places, I realise it's not a great name, so perhaps we could make that a bit clearer. Do you have any suggestions?

src/runtimes/node/bundlers/nft/index.ts Show resolved Hide resolved
Copy link
Contributor

@lukasholzer lukasholzer left a comment

Choose a reason for hiding this comment

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

LGTM, just some comments out of curiosity but nothing major or blocking

@eduardoboucas eduardoboucas merged commit 9d3cc9a into main Aug 1, 2023
10 checks passed
@eduardoboucas eduardoboucas deleted the feat/nft-typescript branch August 1, 2023 12:49
Skn0tt pushed a commit to netlify/build that referenced this pull request May 21, 2024
)

* feat: add support for TypeScript to NFT

* feat: add rewrites

* refactor: stop using esbuild for v2 functions
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants