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

map input source map #1626

Merged
merged 22 commits into from
Oct 7, 2023
Merged

map input source map #1626

merged 22 commits into from
Oct 7, 2023

Conversation

Ka0o0
Copy link
Contributor

@Ka0o0 Ka0o0 commented Aug 28, 2023

Reopening of #1367
Resolves #1457

Since there was a question about the performance in the original PR: this won't cause any performance regressions for the cases where ts-loader is the first loader, so basically for any plain .ts file. For the other cases I would say that functionality goes over performance, since it's currently a serious problem to not have input source maps handled.

@johnnyreilly
Copy link
Member

johnnyreilly commented Aug 28, 2023

this won't cause any performance regressions for the cases where ts-loader is the first loader,

Can you explain why this is the case please?

Also this PR doesn't do async the "webpack way": https://webpack.js.org/api/loaders/#asynchronous-loaders

A single result can be returned in sync mode. For multiple results the this.callback() must be called. In async mode this.async() must be called to indicate that the loader runner should wait for an asynchronous result. It returns this.callback(). Then the loader must return undefined and call that callback.

Significantly, use of async / await in webpack loaders isn't supported to the best of my knowledge

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Aug 28, 2023

this won't cause any performance regressions for the cases where ts-loader is the first loader,

Can you explain why this is the case please?

We only need to call the source-map lib if we have an input source map, otherwise we can do an early return, see this here: https://github.com/TypeStrong/ts-loader/pull/1626/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R685
I've added a comment to highlight this.

Also this PR doesn't do async the "webpack way": https://webpack.js.org/api/loaders/#asynchronous-loaders

A single result can be returned in sync mode. For multiple results the this.callback() must be called. In async mode this.async() must be called to indicate that the loader runner should wait for an asynchronous result. It returns this.callback(). Then the loader must return undefined and call that callback.

Significantly, use of async / await in webpack loaders isn't supported to the best of my knowledge

Yes you are right, the webpack loader interface doesn't support the await/async syntax. In this PR the only method that got changed to an async from a sync method is the makeSourceMapAndFinish method. Now, it will immediately return and evaluate in the "background" which is fine since it anyway already had a reference to the callback method.
The async/await syntax in this case is only syntactic sugar. If you want, I can also remove that.

@johnnyreilly
Copy link
Member

johnnyreilly commented Aug 28, 2023

If you could convert the code to make it do async in the webpack style I'll take a look. I'm on the fence with this feature tbh but I will consider it. It involves taking another dependency which I'm generally hesitant about

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Aug 28, 2023

Great thanks! I've now removed the usage of async/await.

Just one note about the dependency. source-map is developed by Mozilla and is even referenced by the @types/webpack npm package, since they use the same source map data structures. In general incredibly many projects rely on this so it's very likely that a user has this dependency somewhere in his project anyway. Also, it has no further dependencies.

@johnnyreilly
Copy link
Member

Also don't you need to .destroy as well?

https://www.npmjs.com/package/source-map#new-sourcemapconsumerrawsourcemap

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Aug 28, 2023

Good point, I've added that.

BTW I won't have time to further work on this PR the next two weeks, please feel free to make any changes.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@johnnyreilly
Copy link
Member

One day we'll get to have the tests fully run

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 20, 2023

Okay, tests pass. I need to update the test pack for TypeScript 5.2. I'm trying to decide if we should add a comparison test for this also. The yarn.lock doesn't seem to have been updated BTW

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Sep 21, 2023

Okay, tests pass. I need to update the test pack for TypeScript 5.2. I'm trying to decide if we should add a comparison test for this also. The yarn.lock doesn't seem to have been updated BTW

okay, so I guess you want to merge the other PR first?

Sorry the lock file slipped through then, I'll update it.

Regarding the test, I would love to add a test but I think for this I would need to have more guidance from you. As I wrote in the original issue, my problem with the current ts-loader is that I'm not able to debug Vue projects since the input source map generated by the vue-loader was discarded. For the test, is it okay to add the vue-loader as another dev dependency?

src/index.ts Outdated Show resolved Hide resolved
@johnnyreilly
Copy link
Member

Yeah I want to get the test pack PR merged first. Getting meaningless CI failures so I'm using this as an excuse to thin the test matrix a bit. It's way too big. Could you update the yarn.lock file please?

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Sep 24, 2023

I've rebased my changes onto main and updated the yarn.lock file. However, if you look at the file you see that yarn touched the whole file and removed the double quotes. Not sure why, I made sure I've had the current devcontainer running and simply ran yarn add source-map. Any idea what I did wrong or is it fine like this?

@johnnyreilly
Copy link
Member

I wouldn't worry - probably just different yarn versions

@johnnyreilly
Copy link
Member

Could you have a go at writing a new comparison test please? You can learn about comparison tests here: https://github.com/TypeStrong/ts-loader/blob/main/test/comparison-tests/README.md

I would think you can copy / paste the existing sourceMaps test as a starting point. Don't worry if it turns out to be problematic. I think I put this test in the "nice to have" camp rather than the "we need this" camp.

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Sep 26, 2023

I've added a comparison test which requires the vue-loader as described in my previous comment. I'm a bit unsure if it's okay to add another dependency, although it's just for the tests in this case...

@johnnyreilly
Copy link
Member

if it's a devDependency then it's not a problem - it's runtime dependencies that we care about.

What you've done looks like a good start - but I wonder if we could do the symLink that currently sits in the package.json in the same place that the others are done?

fs.symlinkSync(path.resolve(paths.testStagingPath, "lib"), path.resolve(paths.testStagingPath, "node_modules/lib"), "junction");

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Oct 2, 2023

Okay I think I actually don't have to link anything really - the thing with npm link was only that it was installing the dependencies of the test case. I've added the installation into the create-and-execute-test.js script (3b6223e). I've also had to update the package and yarn lock files and the expected output.

@johnnyreilly
Copy link
Member

Sorry this keeps slipping my mind.

Do you want to update the version number in the package.json and add an entry to the CHANGELOG.md please? https://github.com/TypeStrong/ts-loader/blob/main/CHANGELOG.md

v9.5.0 seems appropriate

@Ka0o0
Copy link
Contributor Author

Ka0o0 commented Oct 6, 2023

No worries, done. Hope the description is okay like this.

@johnnyreilly johnnyreilly merged commit 9315855 into TypeStrong:main Oct 7, 2023
43 checks passed
@johnnyreilly
Copy link
Member

Released! Thanks for your work! https://github.com/TypeStrong/ts-loader/releases/tag/v9.5.0

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.

Questions about sourcemap
2 participants