TypeScript and ES6+ support using esbuild. #3738

szkiba · 2024-05-13T18:10:30Z

What?

With the introduction of the "experimental_enhanced" compatibility mode, the test source code is transformed using esbuild instead of Babel.

Why?

Source files with the extension ".ts" are loaded by esbuild's TypeScript loader, which results in partial TypeScript support. This esbuild removes exactly the type information, but does not provide type safety.

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (make lint) and all checks pass.
I have run tests locally (make tests) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#3703

Closes #3703

With the introduction of the "enhanced" compatibility mode, the test source code is transformed using esbuild instead of Babel. Source files with the extension ".ts" are loaded by esbuild's TypeScript loader, which results in partial TypeScript support. This esbuild removes exactly the type information, but does not provide type safety. Source files other than ".ts" are loaded by esbuild's JavaScript loader, which results in the support of a more modern JavaScript dialect than goja.

…rors. Running the tc39 tests also in "enhanced" (esbuild) compatibility mode. Previously, the test wrote the JSON format file to be saved to the standard output, which had to be manually cleaned from the other test output. This solution is difficult to apply in the case of one or more compatibility modes, so it has been changed to the usual golden file pattern. The reference breaking_test_errors-extended.json and breaking_test_errors-enhanced.json files can be updated using the go test -update command.

codecov-commenter · 2024-05-13T18:20:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.90%. Comparing base (befc4d5) to head (1127232).

❗ Current head 1127232 differs from pull request most recent head 01c794d

Please upload reports for the commit 01c794d to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3738      +/-   ##
==========================================
+ Coverage   70.84%   70.90%   +0.05%     
==========================================
  Files         289      290       +1     
  Lines       21156    21191      +35     
==========================================
+ Hits        14988    15025      +37     
+ Misses       5211     5209       -2     
  Partials      957      957

Flag	Coverage Δ
ubuntu	`70.84% <100.00%> (+0.06%)`	⬆️
windows	`70.76% <100.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…urcemap work

codebien

Source files with the extension ".ts" are loaded by esbuild's TypeScript loader, which results in partial TypeScript support. This esbuild removes exactly the type information, but does not provide type safety.

@szkiba can you expand on this topic? Isn't the main advantage of having TypeScript? Does it mean we are delegating it to another tool and k6 makes the assumption to receive a valid script?

With the introduction of the "enhanced" compatibility mode

Can you explain the reason behind the enhanced term, please? Isn't better for clarity to use something related to typescript or esbuild?

Last point, I see we are not running the js/tc39 suite. I expect it is the first requirement for being able to merge this pull request. Can you please make the changes for running it with the new mode? It is probably the fastest way to catch any potential issue without manually checking all the bits.

szkiba · 2024-05-21T10:03:56Z

Hi @codebien , thank you for your review.

@szkiba can you expand on this topic? Isn't the main advantage of having TypeScript? Does it mean we are delegating it to another tool and k6 makes the assumption to receive a valid script?

Sorry if I was unclear: we do not delegate the loading to a tool, but to the esbuild library. Within the esbuild library, there are so-called loaders for each type of content. The quoted sentence indicates that files with the .ts extension will be treated as TypeScript files, in the case of other file extensions, the input will be assumed to be JavaScript.

Can you explain the reason behind the enhanced term, please? Isn't better for clarity to use something related to typescript or esbuild?

The name of the mode is not typescript, because it can also be used with JavaScript files. It's not esbuild because I followed the naming convention, the extended compatibility mode isn't called babel either. In enhanced compatibility mode, esbuild replaces babel.

Last point, I see we are not running the js/tc39 suite. I expect it is the first requirement for being able to merge this pull request. Can you please make the changes for running it with the new mode? It is probably the fastest way to catch any potential issue without manually checking all the bits.

I think the tc39 test is running, I modified the tc39_test.go file:
https://github.com/grafana/k6/pull/3738/files#diff-a1d828027ee2f0cea82c600f5a52b13f91091474fcfc7f2d381db0994363ed62R768

codebien · 2024-05-21T10:23:05Z

The name of the mode is not typescript, because it can also be used with JavaScript files. It's not esbuild because I followed the naming convention, the extended compatibility mode isn't called babel either. In enhanced compatibility mode, esbuild replaces babel.

If this is the case, what is missing for attempting a Babel replacement? So, instead of creating a new mode, we do an expansion of the extended mode.

I think the tc39 test is running, I modified the tc39_test.go file:

Oh, thanks. I missed it.

szkiba · 2024-05-21T10:53:16Z

If this is the case, what is missing for attempting a Babel replacement? So, instead of creating a new mode, we do an expansion of the extended mode.

That's exactly what I think, esbuild will replace babel (one day). I created a new compatibility mode so that esbuild can be completely disabled (and disabled by default). With such a major, potential incompatibility change, I think this is important. Later, esbuild can take over the role of babel and then the two compatibility modes (extended and enhanced) can mean the same thing.

codebien · 2024-05-21T12:54:16Z

If this is the case, should we go with something like experimental-extended-v2?

szkiba · 2024-05-21T13:00:18Z

If this is the case, should we go with something like experimental-extended-v2?

How about experimental-enhanced ? And we will simply remove this if we think that esbuild can replace babel. Otherwise, it can renamed to enhanced if we want to keep both.

pablochacin · 2024-05-21T13:34:46Z

@szkiba I tried running a tests in grafana-api-test library but I get this error:

time="2024-05-21T13:09:45+02:00" level=error msg="GoError: The moduleSpecifier \"../../lib/session\" couldn't be found on local disk. Make sure that you've specified the right path to the file. If you're running k6 using the Docker image make sure you have mounted the local directory (-v /local/path/:/inside/docker/path) containing your script and modules so that they're accessible by k6 from inside of the container, see https://grafana.com/docs/k6/latest/using-k6/modules/#using-local-modules-with-docker.\n\tat go.k6.io/k6/js.(*requireImpl).require-fm (native)\n\tat file:///home/pablo/go/src/github.com/grafana/grafana-api-tests/tests/dashboards/dashboard_read.ts:1:27(61)\n" hint="script exception"

szkiba · 2024-05-21T14:16:18Z

@szkiba I tried running a tests in grafana-api-test library but I get this error:

time="2024-05-21T13:09:45+02:00" level=error msg="GoError: The moduleSpecifier \"../../lib/session\" couldn't be found on local disk. Make sure that you've specified the right path to the file. If you're running k6 using the Docker image make sure you have mounted the local directory (-v /local/path/:/inside/docker/path) containing your script and modules so that they're accessible by k6 from inside of the container, see https://grafana.com/docs/k6/latest/using-k6/modules/#using-local-modules-with-docker.\n\tat go.k6.io/k6/js.(*requireImpl).require-fm (native)\n\tat file:///home/pablo/go/src/github.com/grafana/grafana-api-tests/tests/dashboards/dashboard_read.ts:1:27(61)\n" hint="script exception"

This is a consequence of the fact that esbuild does not run as a bundler, but transforms per module. The module resolving (require function) is implemented by the k6 runtime. Specifying the file extension is mandatory in this implementation.
Currently, the workaround is to specify the file extension in the import.
In the future, it should be considered to modify the implementation of the require function so that the file name extension is not mandatory.

pablochacin · 2024-05-21T14:33:24Z

Currently, the workaround is to specify the file extension in the import.

But the convention is not to include the extension, right?

mstoykov · 2024-05-27T15:16:45Z

js/compiler/enhanced.go

+	opts := api.TransformOptions{
+		Sourcefile:     filename,
+		Loader:         api.LoaderJS,
+		Target:         api.ES2017,


This makes esbuild unneedingly transform a whole bunch of code that goja supports.

In practice from the list of features on https://esbuild.github.io/content-types/#javascript

The only ones not supported are async generators, async iteration and ??=. And in my test the async generator and async iteration doesn't work with the example from mdn.

This also adds a bunch of esnext stuff which I haven't tested, but will argue are probably even less desired by even our less average users given that they aren't released yet.

If you will likely to enable any of those it will be nice to also enable their tc39 tests respectfully. I would recommend that this is done after the initial release of this instead of now.

So my strong recommendation is this to be set to ESNext - which in practice disables esbuild doing any transformation for features and leave just the TypeScript + commonJS transofmration. Some tc39 tests get slightly different results witht that.

You can use the Supported field to set which transformation you want to add. Again I will recommend doing this after this is merged, maybe even after we have had some feedback.

Done, I changed the target to ESNext (and then adjusted the golden file of the tc39 test)

mstoykov · 2024-05-27T15:23:40Z

js/tc39/breaking_test_errors-extended.json

@@ -29,10 +29,10 @@
  "test/built-ins/RegExp/unicode_restricted_octal_escape.js-strict:true": "test/built-ins/RegExp/unicode_restricted_octal_escape.js: Test262Error: RegExp(\"\\1\", \"u\"):  Expected a SyntaxError to be thrown but no exception was thrown at all <at omitted>",
  "test/built-ins/RegExp/unicode_restricted_quantifiable_assertion.js-strict:true": "test/built-ins/RegExp/unicode_restricted_quantifiable_assertion.js: Test262Error: RegExp(\"(?=.)*\", \"u\"):  Expected a SyntaxError to be thrown but no exception was thrown at all <at omitted>",
  "test/built-ins/TypedArrayConstructors/BigUint64Array/is-a-constructor.js-strict:true": "test/built-ins/TypedArrayConstructors/BigUint64Array/is-a-constructor.js: ReferenceError: BigUint64Array is not defined <at omitted>",
-  "test/language/comments/hashbang/function-constructor.js-strict:true": "test/language/comments/hashbang/function-constructor.js: SyntaxError: test/language/comments/hashbang/function-constructor.js: Unexpected token (17:47)\n  15 | const AsyncFunction = (async function (){}).constructor;\n  16 | const GeneratorFunction = (function *(){}).constructor;\n> 17 | const AsyncGeneratorFunction = (async function *(){}).constructor;\n     |                                                ^\n  18 | for (const ctor of [\n  19 |   Function,\n  20 |   AsyncFunction, <at omitted>",
+  "test/language/comments/hashbang/function-constructor.js-strict:true": "test/language/comments/hashbang/function-constructor.js: SyntaxError: Async generators are not supported yet <at omitted>",


I guess this is due to some change in tc39_test.go - most likely because this is the error from babel before and the newer one is from goja.

I don't think this matters in this case - just mentioning it for others.

mstoykov · 2024-05-27T15:39:59Z

But the convention is not to include the extension, right?

This hasn't been the convetion with ESM ... at all.

Node docs

Relative specifiers like './startup.js' or '../config.mjs'. They refer to a path relative to the location of the importing file. The file extension is always necessary for these.

Deno docs

Also, note that we include the full file extension when importing modules, much as you would in the browser. There is also no special handling of files named index.js.

Browsers have always done this.

require in node (and anythign compatible with it ) follows and algorithm to figure out what the user actually meant by probing the filesystem.

This was also one of Ryan Dahl regrets with node. The video is from the middle of a bunch of regrets aroudn how node does modules and resolution and such.

szkiba · 2024-05-27T17:22:08Z

@mstoykov , @codebien, Do I understand correctly that if I rename the "enhanced" compatibility mode to "experimental-enhanced", then this PR will be mergeable?

mstoykov · 2024-05-28T08:02:03Z

I would argue #3738 (comment) is also a fairly good idea. But otherwise I don't see anything else

szkiba · 2024-05-28T08:57:53Z

I would argue #3738 (comment) is also a fairly good idea.

Sure, but you suggested to do it after the merge (or after releasing and get some feedback)

But otherwise I don't see anything else

Thank you, I'll rename the enhanced mode to experimental-enhanced

mstoykov · 2024-05-28T09:09:04Z

The recommended after merging and feedback was about enabling back some of the transformation, not moving to ESNext

szkiba · 2024-05-28T09:16:49Z

The recommended after merging and feedback was about enabling back some of the transformation, not moving to ESNext

Oh, I see, sorry

…ced"

…ransformation for features and leave just the TypeScript + commonJS transformation.

js/compiler/compiler_test.go

cmd/runtime_options_test.go

codebien

LGTM.

@szkiba can you report what is the new binary size with esbuild dependency included, please?

szkiba · 2024-05-28T16:35:20Z

LGTM.

@szkiba can you report what is the new binary size with esbuild dependency included, please?

On master, the k6 binary size is 39612416 bytes, on branch with esbuild the k6 binary size is 43503616 bytes (x86 linux).
The increment is 3891200 bytes ~ 3.9M

szkiba added 2 commits May 13, 2024 19:59

szkiba linked an issue May 13, 2024 that may be closed by this pull request

Native TypeScript support #3703

Closed

szkiba mentioned this pull request May 13, 2024

Native TypeScript support #3703

Closed

szkiba added 8 commits May 14, 2024 06:37

test: increase test coverage

2758044

Merge branch 'master' into feature/3703-typescript-support

ecd287f

fix: Include sources in source map

70eec19

test: test script for source map testing

f3dfb30

Add a magic inline sourcemap URL to the generated code to make the so…

61e193c

…urcemap work

test: Full breaking_test_errors-*.json file generation with -update flag

21712b4

fix: Only use the magic source map URL if source map is enabled

9a5dc14

test: Add test for magic source map URL workaround

e760a20

szkiba marked this pull request as ready for review May 14, 2024 07:36

szkiba requested a review from a team as a code owner May 14, 2024 07:36

szkiba requested review from codebien and olegbespalov and removed request for a team May 14, 2024 07:36

codebien reviewed May 21, 2024

View reviewed changes

codebien requested a review from mstoykov May 21, 2024 12:54

szkiba added 2 commits May 22, 2024 18:43

Merge branch 'master' into feature/3703-typescript-support

daf55ee

Merge branch 'master' into feature/3703-typescript-support

93048e5

mstoykov reviewed May 27, 2024

View reviewed changes

szkiba added 3 commits May 28, 2024 13:40

refactor: rename "enhanced" compatibility mode to "experimental_enhan…

0fc2729

…ced"

refactor: rename "enhanced" compatibility mode to "experimental_enhan…

be63c34

…ced"

Set target to ESNext - which in practice disables esbuild doing any t…

d3f8987

…ransformation for features and leave just the TypeScript + commonJS transformation.

codebien reviewed May 28, 2024

View reviewed changes

js/compiler/compiler_test.go Outdated Show resolved Hide resolved

cmd/runtime_options_test.go Show resolved Hide resolved

test: Move tests related to enhanced mode to enhanced_test.go

01c794d

szkiba requested review from codebien and mstoykov May 28, 2024 14:49

codebien approved these changes May 28, 2024

View reviewed changes

mstoykov approved these changes May 29, 2024

View reviewed changes

szkiba merged commit bdbe5b5 into master May 29, 2024
23 checks passed

szkiba deleted the feature/3703-typescript-support branch May 29, 2024 08:55

mstoykov mentioned this pull request May 29, 2024

feat: Add native TypeScript support using k6pack library #3710

Closed

5 tasks

joanlopez added this to the v0.52.0 milestone Jun 4, 2024

joanlopez mentioned this pull request Jun 21, 2024

Document 'experimental_enhanced' compatibility mode grafana/k6-docs#1630

Merged

11 tasks

BrewTestBot mentioned this pull request Jun 25, 2024

k6 0.52.0 Homebrew/homebrew-core#175582

Merged

andrewslotin mentioned this pull request Jul 31, 2024

feat: added extension list grafana/awesome-k6#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript and ES6+ support using esbuild. #3738

TypeScript and ES6+ support using esbuild. #3738

szkiba commented May 13, 2024 •

edited

Loading

codecov-commenter commented May 13, 2024 •

edited

Loading

codebien left a comment

szkiba commented May 21, 2024 •

edited

Loading

codebien commented May 21, 2024

szkiba commented May 21, 2024

codebien commented May 21, 2024

szkiba commented May 21, 2024

pablochacin commented May 21, 2024

szkiba commented May 21, 2024

pablochacin commented May 21, 2024 •

edited

Loading

mstoykov May 27, 2024

szkiba May 28, 2024

mstoykov May 27, 2024

mstoykov commented May 27, 2024 •

edited

Loading

szkiba commented May 27, 2024

mstoykov commented May 28, 2024

szkiba commented May 28, 2024

mstoykov commented May 28, 2024

szkiba commented May 28, 2024

codebien left a comment

szkiba commented May 28, 2024 •

edited

Loading

TypeScript and ES6+ support using esbuild. #3738

TypeScript and ES6+ support using esbuild. #3738

Conversation

szkiba commented May 13, 2024 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

codecov-commenter commented May 13, 2024 • edited Loading

Codecov Report

codebien left a comment

Choose a reason for hiding this comment

szkiba commented May 21, 2024 • edited Loading

codebien commented May 21, 2024

szkiba commented May 21, 2024

codebien commented May 21, 2024

szkiba commented May 21, 2024

pablochacin commented May 21, 2024

szkiba commented May 21, 2024

pablochacin commented May 21, 2024 • edited Loading

mstoykov May 27, 2024

Choose a reason for hiding this comment

szkiba May 28, 2024

Choose a reason for hiding this comment

mstoykov May 27, 2024

Choose a reason for hiding this comment

mstoykov commented May 27, 2024 • edited Loading

szkiba commented May 27, 2024

mstoykov commented May 28, 2024

szkiba commented May 28, 2024

mstoykov commented May 28, 2024

szkiba commented May 28, 2024

codebien left a comment

Choose a reason for hiding this comment

szkiba commented May 28, 2024 • edited Loading

szkiba commented May 13, 2024 •

edited

Loading

codecov-commenter commented May 13, 2024 •

edited

Loading

szkiba commented May 21, 2024 •

edited

Loading

pablochacin commented May 21, 2024 •

edited

Loading

mstoykov commented May 27, 2024 •

edited

Loading

szkiba commented May 28, 2024 •

edited

Loading