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

Tc39 + js/compiler changes #3831

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Tc39 + js/compiler changes #3831

merged 2 commits into from
Jul 8, 2024

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Jul 4, 2024

What?

Backporting changes from #3456 to make it smaller

Why?

Making that PR smaller makes it easier to review and potentially revert if needed.

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)

@mstoykov mstoykov added this to the v0.53.0 milestone Jul 4, 2024
@mstoykov mstoykov requested a review from a team as a code owner July 4, 2024 16:32
@mstoykov mstoykov requested review from codebien and joanlopez and removed request for a team July 4, 2024 16:32
codebien
codebien previously approved these changes Jul 4, 2024
js/compiler/compiler.go Outdated Show resolved Hide resolved
The changes revolve around moving away from compiling to mostly parsing
the provided code.

This will be relevant in ESM branch when only the parsing needs to be
done for native ESM modules.

For now it mostly adds some noise, but will make that PR a lot better.

The changes to tc39 are mostly to make it nicer as well.

The tc39 changes are quite strange as they definitely have *never*
worked and do not work in k6 currently, so I have no idea what is
happening there.

It is very likely some error shadowing has been exposed.
@@ -70,9 +70,11 @@ func TestCompile(t *testing.T) {
t.Parallel()
c := New(testutils.NewLogger(t))
src := `1+(function() { return 2; })()`
pgm, code, err := c.Compile(src, "script.js", true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit; As the method is name Parse now, should we rename the test name to TestParse? 🤔

@mstoykov mstoykov merged commit a3e64d8 into master Jul 8, 2024
23 checks passed
@mstoykov mstoykov deleted the tc39Changes branch July 8, 2024 10:09
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.

3 participants