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

update esbuild to work with babel #449

Closed
wants to merge 1 commit into from

Conversation

ldrobner
Copy link

@ldrobner ldrobner commented Jun 4, 2023

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

  1. 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.


  1. 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.


  1. 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
Copy link
Member

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.

@dumbmatter dumbmatter closed this Jun 4, 2023
@ldrobner
Copy link
Author

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

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

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