update esbuild to work with babel

ldrobner commented 1 year ago

Summary

It was noticed that errors were being improperly reported when trying to debug the code. This was due to an issue with the esbuild configuration, and the babel plugin callbacks used during bundling.

See the discussion here

Description

It was noticed that any files that used babel to inline bySport or isSport configurations were being listed as unknown in the debugger. It was also noticed that these babel transformed files were unlisted within the debugger.

To solved this issue, a onResolve callback was added to the esbuild plugin function defined in the configuration.

Once the onResolve callback was added, we needed to make sure that the files being filtered by the callback were transformed properly.

To solve this problem, we added a namespace field to the onResolve callback result, and filtered files that passed through the onLoad callback to files within the namespace set during onResolve.

Finally, as good practice, dead code was removed from the onLoad callback.

Now that only certain files were being passed through the onLoad callback, we no longer need to check if the file should be babel transformed. We removed logic to filter out files to be transformed or not, since the only files coming through to the onLoad callback need to be transformed.

dumbmatter commented 1 year ago

Effectively this branch is the same as getting rid of pluginSportFunctions entirely. So source maps work for errors introduced in Dashboard.tsx (or wherever) because Dashboard.tsx is not being processed by Babel at all, it's just sent directly to esbuild.

The onResolve callback triggers only for isSport.ts and bySport.ts. Then it puts those files in the "by-sport" namespace. Then the onLoad callback looks only for files in the "by-sport" namespace, so it only runs on the contents of isSports.ts and bySport.ts. And those files don't contain anything that is transformed by babelPluginSportFunctions, so running Babel on those files will not really do anything.

So what about all the other files with isSport and bySport function calls in them? Those are the ones that are supposed to be transformed by babelPluginSportFunctions. But in your branch, they are not, so the isSport and bySport functions remain in them and are executed in the browser.

Which raises the question... why run this plugin in dev mode at all? It's because of bySport, the way I wrote it means that all the expressions get evaluated, which sometimes causes problems. Like:

const foo = (x: string) => { console.log(x); }
bySport({
    baseball: foo("baseball"),
    basketball: foo("basketball"),
    football: foo("football"),
    hockey: foo("hockey"),
});

With no transformation of bySport to replace that block of code with just foo("basketball");, then all 4 sports will be logged to the console. Logging to the console is innocuous of course, but sometimes there is code in there that causes a runtime error for another sport.

Possibly a better way to do this would be to wrap everything in a function:

const foo = (x: string) => { console.log(x); }
bySport({
    baseball: () => foo("baseball"),
    basketball: () => foo("basketball"),
    football: () => foo("football"),
    hockey: () => foo("hockey"),
});

Then I could run it in dev mode with no transformation. The reason I didn't do this is because I don't want all those extra functions. Both because it makes it uglier to look at, and because then I'd either have to leave an IIFE in the final code (which does add some overhead) or make my plugin even more complicated to remove the function wrapper (and then have to worry about if there is some situation where it behaves differently with/without the function wrapper... I'm not sure, maybe?).

Anyway, the point is that currently the Babel plugin is needed for dev mode and I'm okay with that.

ldrobner commented 1 year ago

@dumbmatter

Took a look into this and I have validated what you are saying. I am simply not transforming files that take advantage of by or is Sport.

Next steps I took was to compare the original to transformed files. Looking at the transformed Dashboard.ts file, it reminded me of this comment and your response.

--

Do you think if we set babel.transformAsync(...{sourceMaps: false}...) we could get the desired functionality?

Could the unknown references be due to overwriting/conflicting sourcemaps? Could the unknown reference be due to bad sourcemaps from babel?

Could the encoding of the sourcemap or maybe just text that is not plaintext js/ts code is causing issues with esbuild?

dumbmatter commented 1 year ago

Do you think if we set babel.transformAsync(...{sourceMaps: false}...) we could get the desired functionality?

No, it would fail for the same reason as setting sourceMaps to true failed.

Could the unknown references be due to overwriting/conflicting sourcemaps? Could the unknown reference be due to bad sourcemaps from babel?

Anything is possible. As I've said before, my understanding is that the source maps in the file should be processed by ESBuild and I guess they are to some extent, the filename just gets lost.

Could the encoding of the sourcemap or maybe just text that is not plaintext js/ts code is causing issues with esbuild?

Doubtful

zengm-games / zengm

update esbuild to work with babel #449

Summary

Description