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

TypeScript and ES6+ support using esbuild. #3738

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

szkiba
Copy link
Contributor

@szkiba szkiba commented May 13, 2024

What?

With the introduction of the "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.

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.

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.
@szkiba szkiba linked an issue May 13, 2024 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.83%. Comparing base (befc4d5) to head (98b071d).

Current head 98b071d differs from pull request most recent head 93048e5

Please upload reports for the commit 93048e5 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3738      +/-   ##
==========================================
+ Coverage   70.78%   70.83%   +0.05%     
==========================================
  Files         287      288       +1     
  Lines       21151    21186      +35     
==========================================
+ Hits        14971    15008      +37     
+ Misses       5217     5215       -2     
  Partials      963      963              
Flag Coverage Δ
ubuntu 70.83% <100.00%> (+0.05%) ⬆️

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.

@szkiba szkiba marked this pull request as ready for review May 14, 2024 07:36
@szkiba szkiba requested a review from a team as a code owner May 14, 2024 07:36
@szkiba szkiba requested review from codebien and olegbespalov and removed request for a team May 14, 2024 07:36
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

szkiba commented May 21, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

szkiba commented May 21, 2024

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
Copy link
Collaborator

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

@codebien codebien requested a review from mstoykov May 21, 2024 12:54
@szkiba
Copy link
Contributor Author

szkiba commented May 21, 2024

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
Copy link

@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
Copy link
Contributor Author

szkiba commented May 21, 2024

@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
Copy link

pablochacin commented May 21, 2024

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

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

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

Successfully merging this pull request may close these issues.

Native TypeScript support
4 participants