Avoid double-reporting the filename in JS library errors/warnings #23787
Conversation
sbc100
force-pushed
the
js_warnings_part2
branch
2 times, most recently
from
02a9d6f to
a440409
Compare
As part of this I also avoid converting JS libraries path to absolute paths where possible. This means that errors and warnings show the actual pathname passed by the users, and not some absolute path. This makes writing tests for the double-reporting issue easier, and makes the errors/warnings easier to read (IMHO).
sbc100
force-pushed
the
js_warnings_part2
branch
from
a440409 to
c5a9b62
Compare
| jslib = utils.path_from_root('src/lib', jslib) | ||
| system_lib = utils.path_from_root('src/lib', jslib) | ||
| if os.path.exists(system_lib): | ||
| jslib = system_lib |
There was a problem hiding this comment.
Does this condition mean that a js library with the same basename as a system one will behave differently than before?
There was a problem hiding this comment.
I think this actually fixes a bug here the lookup/resolution used by get_js_sym_info was different to the lookup/resolution used in src/modules.mjs:
The two codepaths now have the same logic, which is good.
There was a problem hiding this comment.
The JS always had (!path.isAbsolute(filename) && fs.existsSync(path.join(systemLibdir, filename)) as the condition.
There was a problem hiding this comment.
Makes sense. Can we add a test for the bugfix?
There was a problem hiding this comment.
Oh man, that would be a very subtle bug to write a fix for. I can give it a go but I'm not sure its worth it.
There was a problem hiding this comment.
Actually I'm not sure its possible since all --js-library args were processed as os.path.abspath below, so I guess we can consider it an inconsistency/bug that was not previously exposed in any way.
There was a problem hiding this comment.
But maybe there is an edge case where for when the user specifies the name of a js-library that happens to match a system library. I'll write test for that.
There was a problem hiding this comment.
If it's complex to test than I agree it's not worth it. I was hoping it was simple...
As part of this I also avoid converting JS libraries path to absolute paths where possible. This means that errors and warnings show the actual pathname passed by the users, and not some absolute path. This makes writing tests for the double-reporting issue easier, and makes the errors/warnings easier to read (IMHO).