webpack / webpack

A bundler for javascript and friends. Packs many modules into a few bundled assets. Code Splitting allows for loading parts of the application on demand. Through "loaders", modules can be CommonJs, AMD, ES6 modules, CSS, Images, JSON, Coffeescript, LESS, ... and your custom stuff.
https://webpack.js.org
MIT License
64.56k stars 8.77k forks source link

Wrong line numbers when processing files after ts-loader #12025

Open jeffrson opened 3 years ago

jeffrson commented 3 years ago

Bug report

What is the current behavior? Webpack in some cases reports errors according to .js files instead of originating .ts files. For the repro below the error is

ERROR in ./startup.ts 8:11-31
Module not found: Error: Can't resolve 'some_file' in 'C:\src\webpack-5\test'

Line number of error is 8 instead of 13 as in startup.ts - line 8 is the corresponding number from the .js file if package would be compiled with typescript.

If there would be a typescript error, the correct line of the error will be output.

If the current behavior is a bug, please provide the steps to reproduce.

https://github.com/jeffrson/webpack-ts-line-numbers.git

call yarn then yarn webpack finally check yarn tsc for comparing startup.js

What is the expected behavior?

should report line 13 and correct columns for file startup.ts

Other relevant information: webpack version: 5.5.1 Node.js version: 14.15.1 Operating System: Windows 10 Additional tools:

alexander-akait commented 3 years ago

I think it is expected, because resolving based on outputed file from loader, but yes, we can improve this, something like:

ERROR in ./startup.ts 8:11-31 (original 13:11-31)
jeffrson commented 3 years ago

In this case it seems to be simple, but in a real project it's quite confusing if you look at the reported line and cannot find the cause.

In theory the transpiled .js could be reported (instead of .ts), but since this does not exist to have a look at for reference this is not quite helpful as well.

alexander-akait commented 3 years ago

Yep, it will be great to improve this

akgupta0777 commented 3 years ago

Assign it to me so that I can try to solve this.

akgupta0777 commented 3 years ago

But which loader are you using to process typescript @jeffrson

jeffrson commented 3 years ago

Just ts-loader

akgupta0777 commented 3 years ago

ts-loader is a custom package created by TypeStrong. Its not a webpack owned loader in Webpack-contrib Github account. I think I should go to ts-loader repo in order to fix this.

webpack-bot commented 3 years ago

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

webpack-bot commented 3 years ago

Issue was closed because of inactivity.

If you think this is still a valid issue, please file a new issue with additional information.

alexander-akait commented 3 years ago

Still valid, we can get real position from source map, if they present

lukepolo commented 3 years ago

Is there a workaround ? Or do we need to wait for this to be completed?

bonjourjoel commented 2 years ago

ts-loader makes wrong source map line/column numbers.

Zhen-dot commented 2 years ago

In case anyone comes across this issue and just wants a temporary workaround, I've added what works for me here

UPDATE A more user-friendly and perhaps more permanent solution here

Anubarak commented 10 months ago

Since the other issue is closed does this mean there won't be any fix? I have the same config as in the examples but the lines are still wrong - however I use Vue if that matters

log101 commented 5 months ago

I had a chance to work a bit on this issue. I'm able to get the original lines and locations of the error using the source map as @alexander-akait advised. I used source-map library for the conversion, but I wonder where is the right place to do it. For example it could be done here for the ModuleNotFoundError:

https://github.com/webpack/webpack/blob/0bc85d127c14347d646f1354a6cbf2d1142905d8/lib/Compilation.js#L2094-L2098

I could update the dependency location (loc property), but it seems like this logic should be implemented for all errors. So my idea is to do this conversion using the failedModule hook. Basically it'll check if the source map is available, if yes it'll do the conversion. Does this make sense?

alexander-akait commented 5 months ago

I think we should do it after loader processing

log101 commented 5 months ago

I've been working on this problem since last week trying to find possible solutions. I'm still familiarizing myself with the codebase so I'm definitely open for feedback. Just to ensure I'm on the same page, I'd like to briefly describe the issue: The problem is the misalignment of dependency locations. Misalignment occurs when a parent module is processed by loaders, changing line numbers. So when deps generate errors, the line numbers does not align with the original file, creating confusion. I have two potential solutions: 1) As @alexander-akait suggested, converting the line numbers after loader processing. This could be implemented in the NormalModules's build function. handleParseResult function inside is called after module is built. Before sorting the deps at line 1109 we could add a mapper that'd convert the line numbers according to the original file using the source map. Such as:

// Check if source map is present
if (this.originalSource().map()) {
    this.dependencies.map(dep => convertLineNumbers(dep))
}

https://github.com/webpack/webpack/blob/0bc85d127c14347d646f1354a6cbf2d1142905d8/lib/NormalModule.js#L1108-L1120

2) Update the deps before ModuleNotFound error is thrown. But this would only fix the problem for one type of error. Before instantiating the ModuleNotFoundError class at line 2094, we could:

dependencies.map(dep => convertLineNumbers(dep))

https://github.com/webpack/webpack/blob/0bc85d127c14347d646f1354a6cbf2d1142905d8/lib/Compilation.js#L2093-L2100

alexander-akait commented 5 months ago

The first solution looks good, but let's it do only for errored modules

log101 commented 5 months ago

In order to confine the fix to errored modules only as @alexander-akait suggested, I deferred updating the error location to the module factorization.

jssuttles commented 1 month ago

This should still be open, right @log101?

log101 commented 1 month ago

@jssuttles I guess so, no PR's referencing this issue has been merged.