MISC: Support JSX, TS, TSX script files #1216

catloversg · 2024-04-15T18:01:16Z

This PR adds support for JSX, TS, TSX script files.

Transform: Babel vs esbuild-wasm vs swc-wasm

Firefox (SpiderMonkey):

Babel is extremely slow.
esbuild-wasm is fast.
swc-wasm is super fast.

Sample log:

start benchmarkBabel
benchmarkBabel: 13433
start benchmarkEsbuildWasm
benchmarkEsbuildWasm: 3629
start benchmarkSwcWasm
benchmarkSwcWasm: 1545

Chrome/Electron app (V8):

Babel and esbuild-wasm are almost the same.
swc-wasm is much faster than the other two.

Sample log:

start benchmarkBabel
benchmarkBabel: 5749.600000023842
start benchmarkEsbuildWasm
benchmarkEsbuildWasm: 5596.600000023842
start benchmarkSwcWasm
benchmarkSwcWasm: 2423.7999999523163

esbuild-wasm has a big downside: its transform function is an asynchronous function. @d0sboots pointed out a race condition in #1173. Babel and swc-wasm have synchronous functions for transforming, so they don't have this problem.

Using swc-wasm has a downside: wasm file of swc-wasm is fairly big (~19.5 MiB). Babel has an advantage in this case: it's relatively small (babel.min.js is ~2.71 MiB) and I already chose it to parse the AST (check the next part).

I chose swc-wasm to transform scripts, but I also added the testing code for Babel (it can be enabled by setting globalThis.forceBabelTransform).

Benchmark code:

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8" />
    <title>test-tools</title>
    <link rel="shortcut icon" href="data:," />
  </head>

  <body>
    <textarea id="babelContent" rows="30" style="width: 100%"></textarea>
    <textarea id="esbuildWasmContent" rows="30" style="width: 100%"></textarea>
    <textarea id="swcWasmContent" rows="30" style="width: 100%"></textarea>
    <script src="./node_modules/@babel/standalone/babel.min.js"></script>
    <script src="./node_modules/esbuild-wasm/lib/browser.min.js"></script>
    <script type="module">
      const script = await (await fetch("./NetscriptFunctions.ts")).text();
      const maxIteration = 100;
      const maxIterationForWarmup = 50;

      // Babel
      const babelOption = { filename: "script.ts", presets: ["typescript"] };
      function benchmarkBabel() {
        // Warm up
        for (let i = 0; i < maxIterationForWarmup; i++) {
          Babel.transform(script, babelOption);
        }
        const start = performance.now();
        console.log("start benchmarkBabel");
        for (let i = 0; i < maxIteration; i++) {
          Babel.transform(script, babelOption);
        }
        console.log("benchmarkBabel:", performance.now() - start);
      }

      // esbuild-wasm
      await esbuild.initialize({
        wasmURL: "./node_modules/esbuild-wasm/esbuild.wasm",
      });
      const esbuildTransform = (input) => {
        return esbuild.transform(input, { loader: "ts" });
      };
      async function benchmarkEsbuildWasm() {
        // Warm up
        for (let i = 0; i < maxIterationForWarmup; i++) {
          await esbuildTransform(script);
        }
        const start = performance.now();
        console.log("start benchmarkEsbuildWasm");
        for (let i = 0; i < maxIteration; i++) {
          await esbuildTransform(script);
        }
        console.log("benchmarkEsbuildWasm:", performance.now() - start);
      }

      // swc-wasm
      import initSwc, { transformSync as swcTransform } from "./node_modules/@swc/wasm-web/wasm-web.js";
      await initSwc();
      const swcOption = {
        jsc: {
          parser: {
            syntax: "typescript",
          },
          target: "es2020",
        },
      };
      function benchmarkSwcWasm() {
        // Warm up
        for (let i = 0; i < maxIterationForWarmup; i++) {
          swcTransform(script, swcOption);
        }
        const start = performance.now();
        console.log("start benchmarkSwcWasm");
        for (let i = 0; i < maxIteration; i++) {
          swcTransform(script, swcOption);
        }
        console.log("benchmarkSwcWasm:", performance.now() - start);
      }

      const transformedScriptByBabel = Babel.transform(script, babelOption);
      const transformedScriptByEsbuildWasm = await esbuildTransform(script);
      const transformedScriptBySwcWasm = await swcTransform(script, swcOption);
      document.getElementById("babelContent").value = transformedScriptByBabel.code;
      document.getElementById("esbuildWasmContent").value = transformedScriptByEsbuildWasm.code;
      document.getElementById("swcWasmContent").value = transformedScriptBySwcWasm.code;

      benchmarkBabel();
      await benchmarkEsbuildWasm();
      benchmarkSwcWasm();
    </script>
  </body>
</html>

@Snarling @d0sboots Please make your decision on which tool we should use.

Parse AST

We have 3 choices:

Use acorn to parse the AST with acorn-typescript plugin. Reuse the walking code that uses acorn-walk.
- Pros: simple. We can reuse everything.
- Cons:
  - acorn-typescript is slow (up to 2X parsing time compared to babel-parser).
  - acorn-typescript has bug(s). It cannot parse this code: let x = 0;x!++;.
  - We cannot disable JSX support of acorn-typescript when parsing TS code. When we parse TS code (not TSX code), acorn-typescript cannot deal with the ambiguous syntax of TypeScript's type assertion and JSX tag: const a = <any> 0. That code is invalid in TSX file but valid in TS file. However, acorn-typescript cannot parse it because the JSX support is always enabled (v1.4.13).
Use babel-parser to parse the AST to Babel AST. Use babel-traverse to walk the AST.
- Pros: built-in support for parsing JSX and TypeScript.
- Cons: we need to rewrite the walking code. It's complicated.
Use babel-parser to parse the AST to estree AST (with the estree plugin). Reuse the walking code that uses acorn-walk with 2 small extensions. I choose this approach because of these reasons:
- Built-in support for parsing JSX and TypeScript.
- No need to rewrite the walking code.

Other changes

Fix a bug that parses module(s) multiple times

Search for !parseQueue.includes(additionalModule) in RamCalculations.ts.
Test case:

main.js:

import { a } from "/lib1";
import { b } from "/lib2";

export async function main(ns) {
}

lib1.js:

import { x } from "/common";
export const a = 0;

lib2.js:

import { x } from "/common";
export const b = 0;

common.js:

export const x = 1;

In this case, common.js is parsed twice.

Fix wrong Import Error message

Avoid parsing AST twice when checking for infinite loop and RAM usage

Check the code of debouncedCodeParsing() in ScriptEditorRoot.tsx.

Remove `CursorPositions.ts`

This file was used long ago to keep track the cursor position in the editor, but it's not used anymore. CursorPositions.saveCursor() is still called when creating a new script (set default cursor position for the new script), but CursorPositions.getCursor() is not called anywhere else.

Caveat

I disable 2 error codes in the editor:

2792: module resolution problem (TS and TSX files). If we write import { something } from /lib, monaco-editor tries to find the module /lib and it won't be able to find it.
17004: missing setup to support JSX syntax (TSX file).

Please ping me on Discord to discuss about these two problems.

d0sboots · 2024-04-16T04:30:15Z

For completeness, I mention another possible approach for parsing the AST: You can transform first (using whatever chosen tool), and then continue to parse using acorn-walk. This eliminates the need for parsing to know about any JSX or TS syntax.

There is also the other approach possible, which is to use Typescript to transform (and maybe parse?) It will be slow for sure, also setting it up is way more complicated than any other approach. The only upside is that you get actual typechecking, instead of just checking the syntactic correctness, but that is a significant upside. (I.e. there isn't really any point to writing typescript in the in-game editor without typechecking, although there's still value in being able to edit other people's TS code in the in-game editor.)

On the balance, I think I'm happy to endorse the tool choices you made.

d0sboots

I only did a high-level pass so far, looking at the general design. Overall, I like it: Keeping things synchronous simplifies a lot.

d0sboots · 2024-04-16T04:40:20Z

src/utils/ScriptTransformer.ts

+ return transformSync(code, {
+ jsc: {
+ parser: parserConfig,
+ target: "es2020",


Although it's usually not recommended, I feel like we might want to target esnext.

I don't have a strong feeling about "es2020" vs "esnext", so I can change it if you insist. I choose it because that's when JS supports some really useful features (?., ??, globalThis, dynamic import(), etc.).

The reason I suggested "esnext" is because it should prevent things from getting polyfilled and prevent complaints about bleeding-edge syntax, and in this context specifically (you are running a compiler inside your browser) it feels like you should let the browser give you errors about syntax it doesn't support (yet), rather than jsc.

Also, because it won't require maintenance over time.

src/NetscriptJSEvaluator.ts

catloversg · 2024-04-16T10:11:58Z

These are my opinions about other approaches and why I don't choose them:

Transform JSX/TS/TSX to JS, then parse with only `acorn-walk`

This incurs a (potential) performance penalty everytime we need to do the RAM check (tons of time when players edit their code in the in-game editor). It's from parse+walk to transform+parse+walk. Transformer also needs to parse the AST internally before it can transform the code, so in this case it's from BabelParse+AcornWalk to SwcParse(internal)+SwcTransform+AcornParse+AcornWalk.

SWC (internal parse + transformation) is fast, so there is a chance that BabelParse is not much different from SwcParse(internal)+SwcTransform+AcornParse. However, I have not done the benchmark, so I cannot guarantee it.

Personally, I'll only choose this approach as the last resort if I cannot handle the AST parse. The transformation in NetscriptJSEvaluator.ts is unavoidable because we need to run the transformed script eventually. That transformed script is also cached so the performance penalty is acceptable. In the RAM check situation, we cannot cache anything because we need to transform new code everytime players edit it (debouncing helps us in this case).

Use TSC to transform+parse+typecheck

I don't think doing the typechecking in our game code gives players reasonable benefit compared to code base complexity + performance penalty. Typechecking happens in 3 situations:

Write non-JS code in an external editor, tranform+typecheck before pushing the transformed JS code to our game via RFA. In this case, we don't need to do anything.
Write non-JS code in the in-game editor, monaco-editor handles the typechecking for us and show players their errors in the editor. They are supposed to fix their errors in the editor before running their script. When they run their script, we just transform their code and run it.
Same as the second situation, but we also do the typechecking after transforming their script. If their script has type error, we show them the error instead of running their code. To be honest, this does not make sense for me. It's not our job to do the typechecking before running their code. They need to fix their error in the in-game editor or external editor. The editor (in-game or external) will do the typechecking for them, not us.

Snarling · 2024-04-16T12:42:38Z

I think something we could consider with these "compiled" script files is not having them constantly perform ram checking and other such functions while the player types out the code. Instead, the player can be given a way to save + compile which would compile the script into native .js and would also update the ram usage. This would also reinforce to the player that these file types are not native code (whereas presenting them without any compilation might confuse players when real world .ts doesn't work that way). That would make the performance question a lot less relevant, as an opaque manual compilation step would be rare and could be a little slow.

How does importing different types of files work? e.g. is a .js file allowed to import from a .ts file?

catloversg · 2024-04-16T14:08:38Z

What will happen if players create non-JS files, edit it, and run command like ls? Do we show them the transformed ones? For example, they run nano script1.ts, add some code, press Save, back to the Terminal and run ls. What will we show them? How do they run it?

It's the same question when players add non-JS files via RFA. When the RFA server pushes script2.ts to BB, what will happen? How do players run it? In current implementation, they just run it with run script2.ts. We handle everything else in the background. When they run ls, we only show them script2.ts. The transformed script is only in the memory. I think a forced manual step will be really bad UX for them, especially when there are a large number of files.

I agree with "not having them constantly perform ram checking and other such functions while the player types out the code". We can do that by not constantly call parseCode() in ScriptEditorRoot.tsx everytime players edit their code.

For your last question, current module resolution algorithm is simple. It checks all possible filenames for the module and use the first one that it found. For example: import { x } from /lib checks the module called lib. It tries ["lib.js", "lib.jsx", "lib.ts", "lib.tsx"] and use the first one that it found. JS code can use TS module and vice versa. I don't want to use a complicated algorithm. Just see the messy situation of NodeJs and TypeScript when they try to support tons of use cases.

d0sboots · 2024-04-17T01:55:55Z

Use TSC to transform+parse+typecheck
I don't think doing the typechecking in our game code gives players reasonable benefit compared to code base complexity + performance penalty. Typechecking happens in 3 situations:

...

Write non-JS code in the in-game editor, monaco-editor handles the typechecking for us and show players their errors in the editor. They are supposed to fix their errors in the editor before running their script. When they run their script, we just transform their code and run it.

Wait, how do you figure? As far as I'm aware, monaco-editor can't do typechecking without integrating with TSC. It might have this built-in by default, but that only means that it's running TSC (internally) already.

In other words, I agree that the primary benefit is getting typechecking in the editor, but I thought additional effort was needed to get that. It's certainly the case that typechecking cannot be done just on a single-file level, so I'm not sure how monaco-editor could do it without some external support...

Edit: Looking at https://www.npmjs.com/package/monaco-typescript, I think I'm right that TSC is being bundled... also:

Note that this project focuses on single-file scenarios and that things like project-isolation, cross-file-features like Rename etc. are outside the scope of this project and not supported.

catloversg · 2024-04-17T04:39:39Z

As you said, monaco-editor "embeds" the typescript compiler, so it can do all the parsing/transforming/typechecking tasks (single file). We don't have to do anything else.

d0sboots · 2024-05-28T00:07:52Z

Ok, this has sat here languishing (due to us, the maintainers) and I'd like to make forward progress.

From my side, I think the core architectural choices are sound. We could maybe make other choices that might be better, or might not, but these seem like they'll work and won't cause headaches down the road. To me.

@Snarling what do you think? Can we get sign-off on the big parts of the overall design, and then I (or you) can look at this more closely, code-wise?

Snarling · 2024-06-03T16:20:43Z

Sorry @d0sboots @catloversg, been a while since I made time to look stuff over.

I do think this is a huge change and it's pretty important that we do it right / right enough that we're not breaking things with future tweaks.

So just an opinion (and probably an unpopular one), I would personally prefer for scripts in "languages" that require compilation to be a completely new category, and for there to be a player-visible compilation step to turn that file into a .js file so it can actually be run in the browser.

But I think the overwhelming majority of people would probably prefer the implementation here, where they can just directly run .tsx/.ts/.jsx files just as if they were native js scripts. So even though it's not how I would do it, I'm not going to object to the core design here.

As far as things we won't really be able to change much later, here's what I can think of just so we're pretty settled on the answers here:

Whether the compiled scripts can be run directly
How imports work between different filetypes. e.g. Can a .js file import a .tsx file? Currently this is allowed in the PR, but it would match reality more closely if .js could only import .js scripts, where compiled scripts can import from any filetype

Most other implementation details can be changed later on without breaking stuff. Since I know my preferred implementation is probably unpopular, I just want to make sure the answers to the questions above are intentional and generally wanted.

For review, a couple things I noticed:

Not sure if it's an issue on my end, but when I checked this out and did npm ci && npm run start:dev, I got a warning:
The editor doesn't seem to be aware of whether the player is using a *sx file, and it doesn't complain about using jsx syntax in a ,ts/.js file, looks like this is due to that specific error being silenced. I think the correct answer there might be to send the correct flag for .tsx/.jsx files instead of ignoring the error for that flag being missing.

catloversg · 2024-06-03T17:56:06Z

Whether the compiled scripts can be run directly

This PR supports run test.ts. All script files (JS, JSX, TS, TSX) can be run directly with run.

How imports work between different filetypes. e.g. Can a .js file import a .tsx file?

I'm not against your suggestion about ".js could only import .js scripts, where compiled scripts can import from any filetype", but I want to hear more opinions about it before adding the new constraint ".js could only import .js scripts".

I also want to clarify the module resolution algorithm, just in case it's not clear in the PR.
This PR uses a very simple module resolution algorithm, and it's intentional. For example, let's say we have import { add } from "/libs". Where do we find /libs? It just checks all possible script extensions and returns the first script that exists on the server, so it just returns libs.js/libs.jsx/libs.ts/libs.tsx, whatever it sees first. This is problematic if there are both libs.js and libs.ts, but I accept it as a "known limitation".
I don't want to use a complex module resolution algorithm, because that's a rabbit hole, and we should never dig into it. These are some links that I checked, and they are the reason that I want to avoid a complex module resolution algorithm:

Critical dependency: the request of a dependency is an expression

I saw that warning, but I could not find a way to solve that problem. I think it's this one: babel/babel#14301.

About JSX in the editor: monaco-editor does not support JSX, so I disable the error code that is related to JSX syntax. AFAIK, there is no "correct flag for .tsx/.jsx files" because JSX syntax is not supported. There are some workarounds in microsoft/monaco-editor#264, but I did not bother trying them.

d0sboots · 2024-06-03T19:17:11Z

So just an opinion (and probably an unpopular one), I would personally prefer for scripts in "languages" that require compilation to be a completely new category, and for there to be a player-visible compilation step to turn that file into a .js file so it can actually be run in the browser.

That would definitely be a design that could work, and it would certainly be easier to implement. I'm going to explore what that design would look like a bit more, so we can compare/contrast.

For external editor workflows, no change would be needed: compilation would take place outside the game, and only .js files would be uploaded. This is the easy case.

For internal editor uses, it gets weirder. We'd need to introduce a new compile command to turn other types of files into .js files. We'd probably need a ns version of compile() as well. .ts files would be a thing like .txt files, where you can't do anything with them directly.

The compile experience would be a bit subpar, since our shell lacks the features of the overall npm ecosystem. For instance, we don't even have file timestamps (although there is a PR for that), so you can't easily tell if something needs to be recompiled.

There's also the redundancy issue: The in-game editor will give errors about your ts, but then you have an extra compile step because... why? I think the real missing piece is "what does an extra compile step buy us," because all I see are downsides.

G4mingJon4s · 2024-06-08T10:14:53Z

First of all, if I'm saying something completely wrong, feel free to tell me otherwise.
Isn't this a bit over-engineered? To me, it looks like we could just strap a transformer and a typescript parser before the acorn walker and have all the freedom we need.
The user would then write files in their format and all of them get handled by the game; there won't be a weird compiled version of the script in the file system.
Like @d0sboots, I feel an extra compile command or ns function isn't great. I already see players being frustrated about having to first type compile and then run every time.

catloversg · 2024-06-08T13:22:39Z

Can you be specific about what is over-engineered? There are 2 main places:

src\NetscriptJSEvaluator.ts: this is where we "process" the module. In this file, we only need to add a transformer before parsing code.
src\Script\RamCalculations.ts: this is the static RAM calculator. We don't need to transform anything here. We only need to change how the parser works by extending the acorn walker.

G4mingJon4s · 2024-06-08T13:44:24Z

You were talking about all of the different parser options and then there were arguments about how importing from different file types and compiling would work.
To me, the solution is pretty simple, use the typescript parser to parse all files, then transform the ast to use the current ast-walker logic. The way the game goes on from there should be discussed else where.

catloversg · 2024-06-08T14:40:48Z

That's not simple as you say:

Transformer:
- Which one?
- How many KB does it add to the bundle?
- Performance?
- Sync vs Async. You can check d0sboots's analysis of the race condition in MISC: allow jsx files #1173.
AST parser:
- Which one?
- How many KB does it add to the bundle?
- Performance?
- Unresolved bugs?
- Do we need to rewrite the walking logic?
The "interaction"/"relationship" between those libraries. For example, the Babel transformer and parser are in the same package (babel-standalone), so choosing them together gives the benefit of bundle size. However, we also need to consider the performance.

There are many of options with their own trade-offs. Please read the first comment carefully to see how many things we need to consider.

G4mingJon4s · 2024-06-08T15:24:14Z

You already mentioned the option I am suggesting before: Use a transformer (my suggestion being typescript) and transpile user code to a single JS format and go on with the existing steps from there. I can see this simple solution having worse performance than the other suggestions, but nothing says we can't improve from a working baseline.

import * as ts from "typescript";

const code = `const test: number = 1 + 2`;
const transpiledCode = ts.transpileModule(code, tsconfig).outputText;

The transpilation itself is literally one line. No need to re-write all of the existing parsing for now.
We can customize what the output will be through a ts-config, which could also be made available to the user.

Using typescript itself would potentially enable us to use other stuff, like the type-checking, generating d.ts files for the editor, etc..

I am in no way saying the other solutions are worse, I'm just saying this would be the simplest solution and we could go from there.

catloversg · 2024-06-08T15:56:35Z

That approach was discussed in this comment: #1216 (comment). Of course, you can disagree with my opinion there. It's fine. We are still discussing multiple approaches.

The Babel approach has the same "easiness":

Transform: 1 line: babel.transform(script, babelOption);
AST parser: add 2 small extensions for acorn and call extendAcornWalkForTypeScriptNodes(walk.base);, extendAcornWalkForJsxNodes(walk.base);. There is no need to rewrite the walking logic. There is no need to transform the script every time the player edits it in the in-game editor (we debounce it, so it's not really the end of the world).

Choosing tsc, babel or any other library will be the decision of the maintainers. I don't settle on any solution.

You said that things are over-engineered, but I have not seen anything that can be "skipped". Can you point out a specific part that we can cut off from this PR and make it simpler without any downside?
Edit: I want to know the exact "In part/file X, we can skip Y because of Z. We don't need to care about W because of Q.". It's easier to improve the PR with that specific information.

G4mingJon4s · 2024-06-08T17:19:11Z

Maybe over-engineered is the wrong word. So, there are a few things that bother me, not only about this PR, but the general way we treat the player scripts:

The actual parsing and the RAM calculation is mixed up. Instead of having one side parse and transform the script in some way and the other side use that parsed stuff for anything it would like, we have this complete mess about parsing while doing the ram calculation. Here, the calculation can take in an AST that has been parsed before, or source-code that hasn't been parsed yet. I would propose for these very different operations to be split up. When a player saves their script, it should be transpiled transformed, whatever and saved. Then, anything that wants to operate on the script can rely on these steps being done. Right now, it's pretty hard for me to add/change anything to this, since it's a big pile of spaghetti.
Is this and this addition really necessary? Why add these to the walker, if the code has been transformed to JS? Either I am misunderstanding the purpose of the script transformer, or the AST walker is extended when it doesn't need to be.

In general, the PR definitely would work. Maybe I feel too strongly about this, but adding/changing to this spaghetti seems like we are going in the wrong direction. It would certainly take a lot of effort, but I would say we should do the following to enable easier contributions to this in the future:

Have one class for the script and everything associated with it. This includes the filename on the server, the source code, the ram cost etc. Any actions taken on this class are managed by itself. The Script class already serves this purpose to a large extend, but some actions taken on the data of the script, like the transforming of the code, are not reflected in it.
Create a clear structure what happens when and where. This includes things like parsing the code on save, saving the parsed code for other usage etc. Having a consistent way of dealing with user code and parsing would make this part of the game much more extendable.
Define one way of parsing and make it isolated from everything else. This is probably already the case, I'm just saying we shouldn't change this in any way.

catloversg · 2024-06-08T18:23:52Z

For your first point:
calculateRamUsage can be called in 2 places:

updateRamUsage (src\Script\Script.ts): we only have the original source code here, no transpiled JS code, no AST.
updateRAM (src\ScriptEditor\ui\ScriptEditorContext.tsx): we have the AST of the original source code here. In fact, the PR parses the AST in debouncedCodeParsing and reuses it to avoid parsing the AST twice (infLoop and updateRAM).
That's why that method accepts 2 different types of input (code string or AST).

When a player saves their script, it should be transpiled transformed, whatever and saved.

This goes back to these design choices:

When the player runs a script directly (run test.ts), in src\NetscriptJSEvaluator.ts: should we transpile and save the result? Let's say the player have test.ts, should we save the transpiled JS code to test.js? If yes, how and where? It's up for debate.
When the player edits their code (after debouncing): should we transpile and save the result? Let's say the player edits it 1000 times, do we transpile+save+parse (without any acorn extension) 1000 times or parse directly (with acorn extension) 1000 times? It's up for debate.

We need to have a decision from the maintainers before trying to do anything that is "for these very different operations to be split up.".

For your second point:
You are assuming that we have the transpiled JS code in src\Script\RamCalculations.ts. As I said above, it's debatable if we should transpile+save+parse or parse directly the TS/JSX/TSX code in the static RAM calculator. The transpiled JS code in generateLoadedModule is only for running the module. It's not saved, so it's not available in the RAM calculator. Even when it's available, it's still useless because the new edited code in the in-game editor is the new code, not the one transpiled in generateLoadedModule.
That's why I make an extensive analysis of multiple approaches. If we choose to parse directly, we don't have the transpiled JS code, and extending acorn is mandatory.

For your last point:

In general, the PR definitely would work. Maybe I feel too strongly about this, but adding/changing to this spaghetti seems like we are going in the wrong direction. It would certainly take a lot of effort, but I would say we should do the following to enable easier contributions to this in the future:

Have one class for the script and everything associated with it. This includes the filename on the server, the source code, the ram cost etc. Any actions taken on this class are managed by itself. The Script class already serves this purpose to a large extend, but some actions taken on the data of the script, like the transforming of the code, are not reflected in it.

Create a clear structure what happens when and where. This includes things like parsing the code on save, saving the parsed code for other usage etc. Having a consistent way of dealing with user code and parsing would make this part of the game much more extendable.

Define one way of parsing and make it isolated from everything else. This is probably already the case, I'm just saying we shouldn't change this in any way.

It sounds good, except for the fact that it's kind of "I will make a big refactor, and it will be perfect this time". I appreciate your suggestion (it's a good one), and I will take on that mission, but only if Snarling or d0sboots demands it. I prefer making many small improvements iteratively on what we have right now over making a big refactor that touches too many things, especially in this complicated PR. That's why I made this PR as simple as possible.

Snarling · 2024-06-10T14:25:54Z

I think realistically if we are allowing players to directly run from .tsx/etc files (probably the right call) instead of manually transpiling, then we should not save filename.js as a visible file for the player, and if there's a reason for us to store the transpiled .js content, then it should be stored as a hidden property on the Script (and that property should not be included in the save file).

As far as whether ram calc is better done as "Parse .tsx content" or "Transpile/transform to js -> Parse .js content," I don't have a strong opinion. I suspect parsing the .tsx directly would probably be more performant, which matters a lot when we are doing the ram calc frequently in the script editor. But if it's pretty close performance wise, then unifying the ram calc and script execution could be worthwhile.

But those sort of hidden implementation details can always be optimized later on. The most important thing to get right in this PR is just the new functionality and how the player interacts with it. I'm good with the overall implementation, even though there are some cases where it doesn't match the reality of how things work outside the game.

d0sboots · 2024-07-08T20:06:03Z

Working on trying to merge this now...

Not sure if it's an issue on my end, but when I checked this out and did npm ci && npm run start:dev, I got a warning:

I saw that warning, but I could not find a way to solve that problem. I think it's this one: babel/babel#14301.

This seems really serious to me. The way our project is configured, warnings are essentially errors, because of how bold they show up when the dev server reloads. We could change the warning behavior as a very last resort, but honestly I'd be more comfortable with switching packages to something other than babel/standalone, or (hopefully) finding some other way of working around it without having to switch.

The editor doesn't seem to be aware of whether the player is using a *sx file, and it doesn't complain about using jsx syntax in a ,ts/.js file, looks like this is due to that specific error being silenced. I think the correct answer there might be to send the correct flag for .tsx/.jsx files instead of ignoring the error for that flag being missing.

I think maybe Snarling was suggesting only silencing the error when you are editing JSX/TSX files, instead of globally?

d0sboots · 2024-07-08T21:36:21Z

I've looked the code over once in the past, and looked it over again now. It's a big change, but at the same time surprisingly small for how much it's doing. Nice job!

Aside from the issues I re-raised, I don't think there are any blockers left. Snarling said he'd personally prefer an explicit-compile model, but also that most people would probably prefer this model and that he's OK with it. I think this is the right model. The architecture seems sound, and decisions about which plugins we are using can be changed later if need be.

Snarling · 2024-07-08T22:30:41Z

I think maybe Snarling was suggesting only silencing the error when you are editing JSX/TSX files, instead of globally?

Yeah that was my thought. Based on the error, I assumed that there actually was a flag that could be sent for jsx/tsx files:

But it looks like monaco just doesn't support JSX natively yet? Which makes that error message really weird.

There might just not be a more elegant way to deal with this for the time being. The player will still get syntax errors in the ram calculation or when running the script if they try using JSX syntax in a .js/ts file.

src/ScriptEditor/ScriptEditor.ts

catloversg · 2024-07-09T14:02:24Z

In the latest commit, I removed 17004 from the list of ignored diagnostic codes. We don't need to worry about warnings/errors in monaco-editor for jsx syntax anymore.

Test code:
sum.js:

export const sum = (a, b) => a + b;

multiply.ts:

export const multiply = (a: number, b: number) => a * b;

libJsx.jsx:

export const LibText = <>LibJsx</>

libTsx.tsx:

export const LibText = <>LibTsx</>

jsx.jsx:

import { LibText } from "/libJsx";
import { sum } from "/sum";
import { multiply } from "/multiply";

/** @param {NS} ns */
export async function main(ns) {
  ns.tprintRaw(LibText);
  ns.tprintRaw(<>From Jsx</>);
  ns.tprint(`sum: ${sum(1, 1)}`);
  ns.tprint(`multiply: ${multiply(2, 3)}`);
}

tsx.tsx:

import { LibText } from "/libTsx";
import { sum } from "/sum";
import { multiply } from "/multiply";

export async function main(ns: NS) {
  ns.tprintRaw(LibText);
  ns.tprintRaw(<>From Tsx</>);
  ns.tprint(`sum: ${sum(1, 1)}`);
  ns.tprint(`multiply: ${multiply(2, 3)}`);
}

catloversg · 2024-07-09T14:19:32Z

About the problem of the warning from babel-standalone: Based on the comment from JLHwung in the linked issue (babel/babel#14301 (comment)), relevant source code in node_modules\@babel\standalone\babel.js, relevant issue (rollup/plugins#1472), PR (rollup/plugins#1472), I think that we can ignore that warning.

If you want to use another library instead of babel-standalone, I'm fine too. An alternative solution:

Use swc-wasm to transpile instead of babel-standalone. Its performance is the best among 3 options, but its wasm file is fairly big (~19.5 MiB).
Find another library to parse the AST to estree AST. I could not find any good library to do this.

d0sboots · 2024-07-09T19:52:45Z

About the problem of the warning from babel-standalone: Based on the comment from JLHwung in the linked issue (babel/babel#14301 (comment)), relevant source code in node_modules\@babel\standalone\babel.js, relevant issue (rollup/plugins#1472), PR (rollup/plugins#1472), I think that we can ignore that warning.

I agree that the warning does not indicate an actual problem. However, that in and if itself is the problem: the way warnings are set up in our project, they appear high severity, always. If we submit this as-is, it will be a continual source of confusion to new developers, and annoyance to existing ones. It will also tend to cause "warning blindness."

So, if we can't prevent the warning from occurring, I think we have only two choices: downgrade/ignore warnings in our project, or switch packages. If we could somehow downgrade/ignore only this warning, that would be ideal, but it seems unlikely.

d0sboots · 2024-07-09T19:56:37Z

If you want to use another library instead of babel-standalone, I'm fine too. An alternative solution:

Use swc-wasm to transpile instead of babel-standalone. Its performance is the best among 3 options, but its wasm file is fairly big (~19.5 MiB).

Aren't we already doing this? swc/wasm-web is already in package.json.

Snarling · 2024-07-09T21:01:13Z

The warning itself may not be anything too serious, but it's not acceptable for this to be what a developer is greeted with in their web browser when loading into their local instance after running npm run start:dev for local testing:

I was able to silence this warning by using a warning filter:

The additional entry I added to the statsConfig (which is used as module.exports.stats in webpack.config.js:

    warningsFilter: ["./node_modules/@babel/standalone/babel.js"],

Something like this would need to be done if we're keeping babel standalone.

tomprince

Doesn't need to be done in this PR, but I wonder if it would make sense to use babel to rewrite imports for us, rather than as a separate step. (We could also use this for JavaScript, at least in the non-legacy case).

src/Documentation/doc/basic/scripts.md

src/utils/ScriptTransformer.ts

catloversg · 2024-07-10T17:27:35Z

I applied Snarling's suggestion to suppress the warning caused by babel-standalone. There is a small problem, though. stats.warningsFilter is deprecated in favor of ignoreWarnings, so we will see this new warning: [DEP_WEBPACK_STATS_WARNINGS_FILTER] DeprecationWarning: config.stats.warningsFilter is deprecated in favor of config.ignoreWarnings. It won't cause problems for the contributors (no overlay error), so it may be OK.

I tried ignoreWarnings but had a small problem, so I did not use it.
This setting does not work:

    stats: statsConfig,
    ignoreWarnings: [
      {
        file: /\.\/node_modules\/@babel\/standalone\/babel\.js/,
      },
    ],

This setting works:

    stats: statsConfig,
    ignoreWarnings: [
      {
        module: /@babel\/standalone/,
        message: /Critical dependency: the request of a dependency is an expression/
      },
    ],

catloversg · 2024-07-10T17:31:43Z

Aren't we already doing this? swc/wasm-web is already in package.json.

It's only there for testing. If we decide to use babel to transform scripts, I'll remove that option and related code.

tomprince · 2024-07-10T17:34:25Z

I applied Snarling's suggestion to suppress the warning caused by babel-standalone. [...]

I pushed an alternative way to supress the warning to catloversg#1. It works by

telling webpack that an import(expr) does not need a warning when loading @babel/standalone.
telling webpack to not generate the extra chunk that dynamic imports generate.

d0sboots · 2024-07-10T21:31:50Z

Either Tom's approach or the amended ignoreWarnings config seem good to me. I'd prefer not to replace a warning with another warning. :D

d0sboots · 2024-07-10T21:39:04Z

Aren't we already doing this? swc/wasm-web is already in package.json.

It's only there for testing. If we decide to use babel to transform scripts, I'll remove that option and related code.

swc/wasm-web is a regular dependency, not a devDependency, so currently it will be bundled with the app I'm pretty sure (even if not used).

catloversg · 2024-07-11T14:11:09Z

swc/wasm-web is a regular dependency, not a devDependency, so currently it will be bundled with the app I'm pretty sure (even if not used).

What I mean is that I'll remove all swc-related things (swc-related code, the @swc/wasm-web" dependency, and the @swc/core dev dependency) if we use Babel to transform scripts.

catloversg · 2024-07-11T14:24:11Z

We don't have any more blockers now. When the maintainers make the final decision on Babel vs. SWC, I'll finalize the code and amend the documentation.

d0sboots · 2024-07-12T06:44:27Z

We don't have any more blockers now. When the maintainers make the final decision on Babel vs. SWC, I'll finalize the code and amend the documentation.

I don't have a strong opinion either way, and furthermore I think there's a decent chance that whatever we choose, it might get switched out in the future due to unforseen (or foreseen) reasons.

I think how it is now is fine, which I think is swc for pure transform? The size comes primarily from the wasm file itself, right? That should mean that it is only network-loaded when needed, i.e. if you are running a ts/jsx file, and thus the size impact for js-only users (the vast majority) is only really for installed Steam size, where that much is nothing.

The main thing is to document the architectural ideas (these are independent of which package we choose), and then the rationale of specific packages is a much smaller footnote.

catloversg · 2024-07-13T14:27:24Z

In order to use SWC, we need to call the async function initSwc. Other SWC functions must be called after that promise is resolved. Currently, I put initSwc in src\ui\LoadingScreen.tsx. It looks like this:

try {
  await initSwc();
  await Engine.load(saveData);
} catch (error) {
  console.error(error);
  ActivateRecoveryMode(error);
  await Engine.load("");
  setLoaded(true);
  return;
}

The wasm file will always need to be loaded. I could not find a good way to make it load-only-when-needed while avoiding the race condition that you mentioned. If you find a good way to do it, just let me know.

d0sboots · 2024-07-14T21:46:41Z

yolo

d0sboots reviewed Apr 16, 2024

View reviewed changes

catloversg force-pushed the pull-request/misc/support-more-script-extensions branch from f1c4242 to ae6b524 Compare May 23, 2024 16:49

catloversg force-pushed the pull-request/misc/support-more-script-extensions branch from 496d4ca to 481e569 Compare June 3, 2024 10:41

shyguy1412 mentioned this pull request Jun 9, 2024

MISC: allow jsx files #1173

Closed

catloversg force-pushed the pull-request/misc/support-more-script-extensions branch from 481e569 to b58a9c8 Compare June 13, 2024 14:49

MISC: Support JSX, TS, TSX script files

89dfa7a

catloversg force-pushed the pull-request/misc/support-more-script-extensions branch from b58a9c8 to 89dfa7a Compare July 8, 2024 10:31

tomprince reviewed Jul 9, 2024

View reviewed changes

src/ScriptEditor/ScriptEditor.ts Outdated Show resolved Hide resolved

Remove 17004 from the list of ignored diagnostic codes

eafec03

tomprince reviewed Jul 9, 2024

View reviewed changes

src/Documentation/doc/basic/scripts.md Show resolved Hide resolved

src/utils/ScriptTransformer.ts Show resolved Hide resolved

Suppress a warning caused by babel-standalone

16e57c8

Use ignoreWarnings instead of warningsFilter

852dfe5

catloversg and others added 2 commits July 14, 2024 23:56

Add/update comments

8268f7d

Merge branch 'dev' into pull-request/misc/support-more-script-extensions

deef701

d0sboots merged commit 864613c into bitburner-official:dev Jul 14, 2024
5 checks passed

G4mingJon4s mentioned this pull request Jul 15, 2024

MISC: Issues with Typescript support #1488

Closed

catloversg deleted the pull-request/misc/support-more-script-extensions branch July 16, 2024 03:48

antoinedube pushed a commit to antoinedube/bitburner-source that referenced this pull request Aug 5, 2024

MISC: Support JSX, TS, TSX script files (bitburner-official#1216)

285730d

MISC: Support JSX, TS, TSX script files #1216

MISC: Support JSX, TS, TSX script files #1216

Conversation

catloversg commented Apr 15, 2024 • edited Loading

Transform: Babel vs esbuild-wasm vs swc-wasm

Parse AST

Other changes

Fix a bug that parses module(s) multiple times

Fix wrong Import Error message

Avoid parsing AST twice when checking for infinite loop and RAM usage

Remove CursorPositions.ts

Caveat

d0sboots commented Apr 16, 2024

d0sboots left a comment

Choose a reason for hiding this comment

d0sboots Apr 16, 2024

Choose a reason for hiding this comment

catloversg Apr 16, 2024

Choose a reason for hiding this comment

d0sboots Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

catloversg commented Apr 16, 2024

Transform JSX/TS/TSX to JS, then parse with only acorn-walk

Use TSC to transform+parse+typecheck

Snarling commented Apr 16, 2024

catloversg commented Apr 16, 2024

d0sboots commented Apr 17, 2024 • edited Loading

catloversg commented Apr 17, 2024

d0sboots commented May 28, 2024

Snarling commented Jun 3, 2024

catloversg commented Jun 3, 2024

d0sboots commented Jun 3, 2024

G4mingJon4s commented Jun 8, 2024

catloversg commented Jun 8, 2024

G4mingJon4s commented Jun 8, 2024

catloversg commented Jun 8, 2024

G4mingJon4s commented Jun 8, 2024

catloversg commented Jun 8, 2024 • edited Loading

G4mingJon4s commented Jun 8, 2024 • edited Loading

catloversg commented Jun 8, 2024 • edited Loading

Snarling commented Jun 10, 2024

d0sboots commented Jul 8, 2024

d0sboots commented Jul 8, 2024

Snarling commented Jul 8, 2024

catloversg commented Jul 9, 2024

catloversg commented Jul 9, 2024

d0sboots commented Jul 9, 2024

d0sboots commented Jul 9, 2024

Snarling commented Jul 9, 2024

tomprince left a comment

Choose a reason for hiding this comment

catloversg commented Jul 10, 2024

catloversg commented Jul 10, 2024

tomprince commented Jul 10, 2024

d0sboots commented Jul 10, 2024

d0sboots commented Jul 10, 2024

catloversg commented Jul 11, 2024

catloversg commented Jul 11, 2024

d0sboots commented Jul 12, 2024 • edited Loading

catloversg commented Jul 13, 2024

d0sboots commented Jul 14, 2024

catloversg commented Apr 15, 2024 •

edited

Loading

Remove `CursorPositions.ts`

d0sboots Jul 8, 2024 •

edited

Loading

Transform JSX/TS/TSX to JS, then parse with only `acorn-walk`

d0sboots commented Apr 17, 2024 •

edited

Loading

catloversg commented Jun 8, 2024 •

edited

Loading

G4mingJon4s commented Jun 8, 2024 •

edited

Loading

catloversg commented Jun 8, 2024 •

edited

Loading

d0sboots commented Jul 12, 2024 •

edited

Loading