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

MISC: allow jsx files #1173

Closed

Conversation

shyguy1412
Copy link
Contributor

This will allow creating jsx files and using jsx syntax inside the bitburner editor.
This would also allow ts/tsx if the parser for ram checking gets extended for that in the future.

Right now I am having issues getting tests to pass tho, since the node enviroment doesnt follow the same specs as browsers.
Code_aweGwEr7pK

@d0sboots
Copy link
Collaborator

Is this the TS change but with the TS/RAM stuff stripped out, essentially? Because I will admit, allowing TS seems a lot more exciting than allowing JSX, although that does have its uses.

// Early return for recursive calls where the script already has a URL
if (script.mod) {
addDependencyInfo(script, seenStack);
return script.mod;
}

const scriptJs = script.filename.endsWith(".js")
? script.code
: //tsx can load js, ts, jsx and tsx.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this is true, tsx is not a strict superset of ts and jsx. I can't find the reference right now, but there are cases where valid TS will be interpreted differently if it's compiled as TSX. Best to use the proper loader based on filetype.

Comment on lines +4 to +14
let resolved = false; //this is ugly but lets us skip awaiting a resolved promise
const initPromise = esbuild
.initialize({
wasmURL,
})
.then(() => (resolved = true));

export const transform: typeof esbuild.transform = async (input, options) => {
if (!resolved) await initPromise;
return esbuild.transform(input, options);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Transform is already a promise-based API, so what's the big deal here?

Why not just

Suggested change
let resolved = false; //this is ugly but lets us skip awaiting a resolved promise
const initPromise = esbuild
.initialize({
wasmURL,
})
.then(() => (resolved = true));
export const transform: typeof esbuild.transform = async (input, options) => {
if (!resolved) await initPromise;
return esbuild.transform(input, options);
};
const initPromise = esbuild.initialize({wasmURL});
export const transform: typeof esbuild.transform = (input, options) => {
return initPromise.then(_ => esbuild.transform(input, options));
};

@@ -76,15 +77,30 @@ function addDependencyInfo(script: Script, seenStack: Script[]) {
* @param scripts array of other scripts on the server
* @param seenStack A stack of scripts that were higher up in the import tree in a recursive call.
*/
function generateLoadedModule(script: Script, scripts: Map<ScriptFilePath, Script>, seenStack: Script[]): LoadedModule {
async function generateLoadedModule(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to think carefully about the implications of this being async. There's a lot of subtle stuff tied up in this.

@d0sboots
Copy link
Collaborator

OK, after thinking about it more I know why I was concerned. Right now there are some invariants that are being upheld due to the synchronous nature of the current code. Let me show you what I mean.

Let's say we have main.js with the following code:

import { getSquare } from "a.js";
import { getSize } from "b.js";

export async function main(ns) {
  ns.tprint(getSize(getSquare(3)) ** 2);
}

a.js:

class Rectangle {
  width;
  height;

  constructor(w, h) {
    this.width = w;
    this.height = h;
  }
}

export getSquare(size) {
  return new Rectangle(size, size);
}

b.js:

export getSize(rect) {
  return Math.max(rect.width, rect.height);
}

This (should) print 9 when executed. However, let's assume the following sequence of events:

  • main.js goes to be compiled
  • generateLoadedModule for main.js
  • generateLoadedModule for a.js
  • Independently, the user saves new code for main.js and b.js. The new code refactors the **2 out of main and changes getSize so that it returns rect.width * rect.height. The resulting code should still print 9.
  • generateLoadedModule for b.js. This will operate on the new version of b.js.
  • Return back out to generateLoadedModule for main.js. This will now transform main.js with the code of the new b.js.

The result is a compiled module that prints 81. This will be persistent; it will not fix itself until something invalidates one of the modules involved.

This scenario is currently impossible, because the generateLoadedModule steps happen synchronously so no actions can happen in-between. However, if they become async then those guarantees go out the window.

The core requirement here is that when a module is compiled, the code that is compiled must be the code that was present at the time of the compile() call. There are a few different ways to approach this, but they have varying levels of both complexity and generalizability.

The easiest and most elegant method is when the imports can be directly parsed without having to wait for a transform step. This is what is possible in this PR with acorn-jsx, but it won't generalize (I don't think?) to TS.

The other approach is to take a snapshot of the scripts state and operate on that.

@shyguy1412
Copy link
Contributor Author

This is now going to be covered by #1216 so I'm closing this

@shyguy1412 shyguy1412 closed this Jun 9, 2024
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.

2 participants