update esbuild to work with babel #448
Conversation
There was a problem hiding this comment.
Your edits have a few errors that prevent this from working, maybe caused by you basing your edits on an old version of esbuildConfig.js, so I didn't actually get to see if it solves the problem. But from looking at the code, I am skeptical it solves the problem because the onResolve callback is probably not doing anything (see the comment there)
Thank you again for your work on this :)
| import babelPluginSportFunctions from "../babel-plugin-sport-functions/index.js"; | ||
| import getSport from "./getSport.js"; |
There was a problem hiding this comment.
Why change these lines? Both of these imports are now referencing file that don't exist, so it's an error when you run it.
There was a problem hiding this comment.
I am not sure what is happening here. Something might have changed when I built, but I was unaware of these changes when I made a commit. I will need to revert this change.
There was a problem hiding this comment.
Generally you should make sure your master branch is updated to the latest version before making a new branch from it. I recommend you do that and then make these changes again on that new branch, since this is not a big PR.
|
|
||
| const babelCache = {}; | ||
|
|
||
| // Use babel to run babel-plugin-sport-functions. This is needed because the way bySport is defined, the sport-specific code will run if it's present, which can produce errors. It's not actually needed for isSport. | ||
| const pluginSportFunctions = nodeEnv => ({ | ||
| name: "sport-functions", | ||
| setup(build) { | ||
| build.onLoad({ filter: /\.tsx?$/, namespace: "file" }, async args => { | ||
| build.onResolve({ filter: /\..{1,}Sport/ }, async args => { |
There was a problem hiding this comment.
The filter runs on the filename, not on the contents of the file, so I think this will never run because no filename matches that regex.
There was a problem hiding this comment.
I believe the on resolve callback will be called when we try to 'resolve' the import. So when an import is found in a file, esbuild will try and inline the code from the imported file using our defined callbacks.
This filter will invoke the callback when we find an import with the path field, in the esbuild arguments, equal to .**Sport. This should handle any import for isSport and bySport, I think.
There might be cases where the path field is not .\isSport or .\bySport. Might be cases where path is ..\bySport, bySport, or something similar that could be missed.
There was a problem hiding this comment.
what do you think this filter should be?
There was a problem hiding this comment.
It's unclear to me what the filter is trying to accomplish. But ultimately, if it works, then I'll figure out why. So make a PR from the latest master so it actually runs and then we'll see :)
| const isTSX = args.path.endsWith("tsx"); | ||
|
|
||
| const loader = isTSX ? "tsx" : "ts"; |
| @@ -69,16 +64,15 @@ const esbuildConfig = ({ nodeEnv, name }) => { | |||
| outfile, | |||
| bundle: true, | |||
| sourcemap: true, | |||
| jsx: "automatic", | |||
| jsxDev: nodeEnv === "development", | |||
| inject: ["tools/lib/react-shim.mjs"], | |||
There was a problem hiding this comment.
This file doesn't exist anymore. Maybe you were basing this on an old version of esbuildConfig.js?
There was a problem hiding this comment.
100% could be the case. I think there is something wrong with my fork. I forked the repo way back in 2019, I am sure lots of changes have been made since.
| // This is needed because dropbox conditionally requries various node builtins, and esbuild chokes on that even though it never actually makes it to the browser. Skip it for the worker though, otherwise that introduces a spurious error because the type export in worker/index.ts results in a module.exports being added. | ||
| platform: name === "ui" ? "node" : undefined, |
There was a problem hiding this comment.
I never had this problem, I am skeptical this change is needed. But the line you deleted is needed, otherwise the build fails for the same reason I wrote in the comment.
There was a problem hiding this comment.
I am not sure what is happening here. Something might have changed when I built, but I was unaware of these changes when I made a commit. I will need to revert this change.
There was a problem hiding this comment.
@dumbmatter when making a second commit to this PR, should I squash my commits?
There was a problem hiding this comment.
Generally if you're going to squash, you do that when merging (so "you" would be me, since I'm the one who can merge). If you do it while the PR is still in development, it makes it harder to review because I can't easily see what changed since the last time I looked at it.
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
bySportorisSportconfigurations were being listed asunknownin the debugger. It was also noticed that these babel transformed files were unlisted within the debugger.To solved this issue, a
onResolvecallback was added to the esbuild plugin function defined in the configuration.Once the
onResolvecallback 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
onResolvecallback result, and filtered files that passed through theonLoadcallback to files within the namespace set duringonResolve.Finally, as good practice, dead code was removed from the
onLoadcallback.Now that only certain files were being passed through the
onLoadcallback, 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.Continuing the Discussion