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

Why are source maps not working right in dev mode? #426

Open
dumbmatter opened this issue Jul 12, 2022 · 19 comments
Open

Why are source maps not working right in dev mode? #426

dumbmatter opened this issue Jul 12, 2022 · 19 comments

Comments

@dumbmatter
Copy link
Member

$200 prize if you can resolve this issue, see below for detail

I made a branch https://github.com/zengm-games/zengm/tree/esbuild-sourcemaps containing a commit f0d524f which throws an error when the main page is loaded. Try it out:

$ git checkout esbuild-sourcemaps
$ yarn install
$ yarn run start-watch

Go to http://localhost:3006 in your browser and observe an error in the console like this:

image

So it thinks the error is on line 211 of src/ui/views/DraftScouting/index.tsx, but actually it's on line 211 of src/ui/views/Dashboard.tsx. It got the line number right, but the file wrong. Seems this happens to any error I introduce anywhere in the codebase - the source maps get the line number right, but the file wrong. Very annoying.

This must have something to do with esbuild, since it happens only in dev where esbuild is used, rather than in prod where rollup is used.

I spent some time poking around to try to figure out what's going on, but I couldn't figure it out.

If someone can tell me how to fix this, I will give you $200. By "fix" I don't mean something crazy like "switch back to using rollup in dev mode, which is like 1000x slower than esbuild".

@sondersocha
Copy link

This must have been fixed? I'm getting the correct filename (../Dashboard.tsx) and line (211) in my developer console.

@dumbmatter
Copy link
Member Author

Hm, maybe it's platform dependent?

I just tried it on a computer with:

  • Ubuntu 20.04
  • yarn 1.22.19
  • node 16.16.0
  • Chrome 104.0.5112.79

and it still looks exactly like the screenshot. Are you on a different OS/browser, or different versions of those tools?

@sondersocha
Copy link

I tried it on:

  • Windows 11
  • yarn 1.22.17
  • node 18.0.0
  • Chrome 104.0.5112.79

I initially tried the most obvious solution, which was to use a different browser. I went over to Firefox and saw the same, correct, error output. Then I upgraded yarn to 1.22.19 and saw the same. Next, I used NVM to switch my active version of Node to match 16.16.0, and still saw the same, correct, error output.

At this point, it seems it could be an OS issue. Fortunately, I also have Ubuntu installed on my machine, so I will now switch over to that and see if it's an OS issue for whatever reason. Give me just a few minutes.

@sondersocha
Copy link

You know what, disregard my last post. I misunderstood your initial post. Mine actually does incorrectly link to the DraftScouting.tsx file as well. What threw me off was it displaying the Dashboard.tsx, but I see that it does that in your initial screenshot as well. Now I understand the problem.

I will continue to mess around with it as I do have a decent amount of experience working with esbuild, which is what drew me to this post. It's certainly puzzling. Hopefully I can stumble upon something for you.

@ldrobner
Copy link

ldrobner commented May 5, 2023

  • Microsoft Windows 11 Pro
  • yarn 1.22.17
  • node 16.16.0
  • User Agent: [Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/112.0.0.0 Safari/537.36 Edg/112.0.1722.68] -- Microsoft Edge built on Chromium (i believe)

My error looks like:

Uncaught Error: FOO
    at Dashboard (unknown:211:8)
    at renderWithHooks (react-dom.development.js:16305:18)
    at mountIndeterminateComponent (react-dom.development.js:20074:13)
    at beginWork (react-dom.development.js:21587:16)
    at HTMLUnknownElement.callCallback2 (react-dom.development.js:4164:14)
    at HTMLUnknownElement.__trace__ (inline-script-content.js:146:30)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:4213:16)
    at invokeGuardedCallback (react-dom.development.js:4277:31)
    at beginWork$1 (react-dom.development.js:27451:7)
    at performUnitOfWork (react-dom.development.js:26560:12)

When I focus on the unkown:211:8 it reads https://localhost:3006/src/ui/views/unknown:211:8


What if we change the sourcemap type? -- https://esbuild.github.io/api/#sourcemap

Just going to run some tests.

tried the 3 other methods listed in the docs

  • inline: same as original
  • both: same as original
  • external: gave location in the bundled JS file.

Switching back to linked


ui.js.map has 271 references to unknown files.

Which files are listed as unknown?

  • We know Dashboard.tsx is one

Why are some files unknown and others not?

What do these unknown file mappings have in common?

  • Similar dependencies?
  • Same file extension?

Which directories have more unknown files?

  • Lets start with a directory like /src/common. It has 53 files, 2 of them are *.test.ts files.
  • 45 references in js.map, 10 to unknown
  • side note: are *.test.ts files bundled with esbuild?

@dumbmatter
Copy link
Member Author

Yeah I see the same error you do now. It is better to say it's in an unknown file rather than say it's in the wrong file, but it'd still be better if it said what the actual file was.

Also I'm not sure if you meant to include more in your previous comment, or if you're going to write another comment? Seems to end abruptly :)

@ldrobner
Copy link

ldrobner commented May 6, 2023

Yes. Sorry I had to run out quickly, never got to finish my last thought.


Do you know which of these files are not bundled? And why are they not bundled? I believe 6 of them are not bundled, but not 100 % on that

  • getCols.ts
  • playThroughInjuriesFactor.ts
  • bySport.ts
  • getBestPlayerBoxScore.ts
  • types.hockey.ts
  • types.baseball.ts
  • filterPlayerStats.ts
  • posRatings.ts
  • teamInfos.ts
  • types.ts
  • types.football.ts
  • polyfills.ts
  • processPlayerStats.ts
  • constants.ts
  • index.ts
  • types.basketball.ts

The types*.ts contain only TypeScript types, not actual JS code. So they don't get bundled.

bySport.ts is just a placeholder for TypeScript, when it's actually compiled it gets replaced by https://github.com/zengm-games/zengm/tree/master/tools/babel-plugin-sport-functions

polyfills.ts are only included in the legacy build, it's not included in the modern build or in dev mode.

The others should be included in the bundle, I think.

That makes a lot of sense. Looks like these 10 files get bundled as unknown:

  1. getCols.ts
  2. playThroughInjuriesFactor.ts
  3. getBestPlayerBoxScore.ts
  4. filterPlayerStats.ts
  5. posRatings.ts
  6. teamInfos.ts
  7. types.ts
  8. processPlayerStats.ts
  9. constants.ts
  10. index.ts

@dumbmatter
Copy link
Member Author

Do you know which of these files are not bundled? And why are they not bundled? I believe 6 of them are not bundled, but not 100 % on that

The types*.ts contain only TypeScript types, not actual JS code. So they don't get bundled.

bySport.ts is just a placeholder for TypeScript, when it's actually compiled it gets replaced by https://github.com/zengm-games/zengm/tree/master/tools/babel-plugin-sport-functions

polyfills.ts are only included in the legacy build, it's not included in the modern build or in dev mode.

The others should be included in the bundle, I think.

@ldrobner
Copy link

ldrobner commented May 6, 2023

I am thinking a lot about bySport.ts.

The ten files we have narrowed down to unknown seem to all use bySport.ts somewhere, except types.ts.


bySport.ts is just a placeholder for TypeScript, when it's actually compiled it gets replaced by https://github.com/zengm-games/zengm/tree/master/tools/babel-plugin-sport-functions

Does this mean that bySport.ts is compiled using the plugins section of the esbuild config?

If so, can the plugin step of compiling have an effect on the final source map?

  • Thinking if the plugin has not been defined yet, the source map is making a reference to an undefined function/location causing a bad reference or no reference.
  • I have not explored the esbuild docs enough to understand the bundling process. This must be something esbuild has thought about.

@dumbmatter
Copy link
Member Author

I think you're right!

Originally I wanted to run that Babel plugin to replace isSport/bySport only in prod, but I realized it's actually required in dev too because the bySport code blocks for different sports all being simultaneously executed can sometimes result in errors. It would be better if they were all wrapped in IIFEs, but that makes the syntax uglier, so I decided to rely on my Babel plugin in dev too.

So anyway, it is possible to delete that plugin, it might break some stuff but most things still work. And deleting the plugin fixes the stack trace!

https://esbuild.github.io/plugins/ has examples of plugins that maintain source maps. IIRC it may have even had a Babel plugin example in the past, similar to what I'm doing. So in theory what I'm doing should work - inline source maps emitted from the plugin. So I'm still not sure why my plugin doesn't work!

Regarding the bounty, does this sound fair? $100 to @ldrobner know for identifying the source of the problem, and $100 in the future if you can figure out how to actually fix it? If so email me jeremy@zengm.com with how you want to be paid, Paypal is easiest but I can probably do others.

@ldrobner
Copy link

ldrobner commented May 9, 2023

Hello. I have been trying to read through some more of the esbuild documentation. Mostly reading about plugin callback functions -- https://esbuild.github.io/plugins


  • Instead of bundling when the file loads (build.onLoad), what would happen if we tried to route on resolve (build.onResolve)? If I am understanding correctly, onLoad is run before adding the file to the sourcemap, onResolve is run after we have added the file to the sourcemap.
  • Maybe we can take advantage of onResolve (or something else) to "reload" the unknown paths. How will we know when we have written a file with an unknown path?
  • bonus: Does pluginSportFunctions get called for all files? Ex: lets say we come to a file that does not need a bySport plugin. Will the plugin function be called?

  • I have also wondered about the source map type matching. How does the sourcemap type set in the plugins (esbuildConfig.mjs::33) work with the sourcemap type set in the generic config (esbuildConfig.mjs::69)?
  • In the generic config we have set sourcemap == true, which means source map is of type linked. In the plugin configuration we have set sourcemap == inline which means source map is of type inline.
  • I had previously tried to solve this issue by changing the general configuration source map type, but I had not tried combinations of plugin and general source map types.
  • Do you think trying different combinations of source map types would be worth it?

@dumbmatter
Copy link
Member Author

Instead of bundling when the file loads (build.onLoad), what would happen if we tried to route on resolve (build.onResolve)? If I am understanding correctly, onLoad is run before adding the file to the sourcemap, onResolve is run after we have added the file to the sourcemap.

I don't think you can process file contents in onResolve.

bonus: Does pluginSportFunctions get called for all files? Ex: lets say we come to a file that does not need a bySport plugin. Will the plugin function be called?

It does, because there's no way to tell if it's needed without looking in the file.

if (result === undefined) {
// Might as well return the text, since we have it in memory already
result = { contents: text, loader };
}
if these lines were deleted then the plugin would do nothing for files without bySport, but I figure that would just make it slower by making it load the same file from disk again later.

I have also wondered about the source map type matching. How does the sourcemap type set in the plugins (esbuildConfig.mjs::33) work with the sourcemap type set in the generic config (esbuildConfig.mjs::69)?

The former is the setting for babel, the latter is the setting for esbuild. So they are different settings with different allowed values. The way it should be working is that babel emits an inline sourcemap in each file it processes, and then esbuild notices those inline sourcemaps and uses that knowledge when creating its final sourcemaps. Something is probably going wrong in one of those processes - either babel is producing a somehow incorrect sourcemap, or esbuild is not correctly parsing/using the sourcemaps from babel. Or I am configuring it wrong, that's also possible, but I don't think I am.

@ldrobner
Copy link

I logged each "exit"/setting of 'result' into a file to see if there was anything different between babel transformed files and the files you simply return the text of.


Printed out a lot of information and I have yet to make sense of all of it, but I am unsure if the babelCache is being used. This could be because we are using mtimems to validate the entry in the cache. I wonder if there is something better to use.

I have not confirmed this theory, but I have been thinking about this line:

if (babelCache[args.path] && babelCache[args.path].mtimeMs === mtimeMs) {

Do you think that babel or esbuild is changing the mtimems field when reading the contents from the file? And would this cause us to write, but not read from the babel cache?


Taking a step back and going back to my original comment

Thinking if the plugin has not been defined yet, the source map is making a reference to an undefined function/location causing a bad reference or no reference.


I am thinking a solution could be setting a strict order of bundling.

My concern is that esbuild already takes care of this. I am unsure how files are read and added to the source map, but my best guess would be a type of post order traversal system. We are passing index.ts(x) to the builder and then it sees import * from directory. We then read directory.files[0]. The builder then starts at the top of the file again and the recursive cycle of bundling imports/dependencies are handled. This would mean that files like bySport and isSport get bundled before they are referenced by other files being added to the source map.

I am going to simplify my logging and try and check the order of which files are bundled.


Also looking at the *.map files created by esbuild. It looks like contents from files like getCols.ts are added to the sourceContent field in the map, but <relative_path>\getCols.ts is not added to the sources field. I will have to check if other babel transformed files are added to the map in a similar fashion.

Wondering if we could using something like a source-base-path feature could help with this issue. -- evanw/esbuild#2554


Let me know if I made any sense here.

@dumbmatter
Copy link
Member Author

Do you think that babel or esbuild is changing the mtimems field when reading the contents from the file? And would this cause us to write, but not read from the babel cache?

mtime is modification time, so reading the file shouldn't change it. In theory there could be a bug, but it'd be a very weird one, because it'd have to be updating mtime without also triggering a rebuild from watching for changed files.

The point of babelCache is for updates, not for the initial run.

Let me know if I made any sense here.

In general, I think my code is pretty mundane, and I think I may have originally copied most of it from the esbuild docs or something similar. So idk, I feel like it should work as is, but clearly it doesn't. The solution probably isn't something super complicated though.

@ldrobner
Copy link

ldrobner commented May 20, 2023

Makes sense.


I have been thinking more about on resolve callbacks. I believe we can use on resolve callbacks to help better set the file's path in the map file. I will try and test some theories and get a PR out.

@ldrobner
Copy link

ldrobner commented May 28, 2023

hello. Sorry for the delay. I tried out 2 more solutions:


  1. add on-Resolve callback / add resolvePath to onLoad callback function
    • Thought that adding this functionality would help esbuild resolve paths and create the correct mapping
    • Tried to resolve .\bySport and .\isSport imports by adding a callback.
      build.onResolve({filter: /\..{1,}Sport/} async args => { return holdingDirectoryPath })
    • I tried to simplify this change by using the resolvePath field in the onBuild results
    • no change in the source map
  2. changed the babel option sourceMaps from inline to true
    • After some more work with esbuild, I figured esbuild was not the problem.
    • Started reading more about babel's transformAsync function. Looked through the options we are using and thought about changing sourceMaps
    • We have 4 different settings for this option
      • false/undefined/null (default) - does nothing?
      • inline - returns the transformed text to place into the source map by our code
      • both - returns both the mapping and content to be inserted into the source map file
      • true - write the content and mapping to the source map file through babel (recommended by babel docs)
    • looks like this option works!
    • Here are the results from changing the babel transform config

image


Would love to commit this, but I get a 403 when trying to. Let me know if this make sense. Happy to commit, etc, just let me know.

@dumbmatter
Copy link
Member Author

Wow, incredible! You are a true hero for figuring this out.

If you want to commit, you have to fork the repo and then make a PR from one of your branches. If you'd like to do that, please go ahead! Otherwise I can make the change myself, but I understand if you want that commit as a trophy for figuring all this out :)

@dumbmatter
Copy link
Member Author

dumbmatter commented May 29, 2023

Actually it's still not solved!

Changing it to true according to https://babeljs.io/docs/options#source-map-options will put the source map in the result object... which as you can see here

const contents = (
await babel.transformAsync(text, {
babelrc: false,
configFile: false,
sourceMaps: "inline",
plugins: [
[babelPluginSyntaxTypescript, { isTSX }],
babelPluginSportFunctions,
],
})
).code;
is not used except for the code property. So in this case, the source map is ignored. It's the same as setting sourcemaps to false.

So if the error happens inside or after a bySport function, the final source map (and therefore the error in the browser console) will point to the right file but the wrong line.

The "inline" setting would include the source map in the code. And to some extent that is working, otherwise it wouldn't get the correct line number. Just the file name is being lost somehow.

@ldrobner
Copy link

ldrobner commented May 31, 2023

Hmm. I think I understand what you are saying.

Why does changing babel.sourcemap to something other than inline is sort of work?

  • We are not returning a sourcemap with the code, so esbuild is mapping the transformed text? This could be why we are seeing wrong line numbers.
  • When the Dashboard.tsx file is not transformed, the FOO error is thrown on line 211. If transformed, FOO happens on 173.

As I messed around with the configuration more, I noticed something... Dashboard.tsx is not in /src/ui/views when I inspect a build with babel.sourcemap = 'inline'.

This makes me want to circle back to onResolve. Seems like esbuild is unable to resolve the path of files that use bySport and isSport which is why they are undefined in the bundle. The inline sourcemap could be why we get the right line number.


I re-added the onResolve callback from before:

build.onResolve({filter: /\..{1,}Sport/} async args => { return holdingDirectoryPath })

And I shortened the onLoad callback to adjust files handled in onResolve.


Checkout the results:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants