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

Sourcemap failing when filename has spaces or is otherwise affected by URL percent-encoding #1322

Closed
edqx opened this issue May 19, 2021 · 15 comments
Milestone

Comments

@edqx
Copy link

edqx commented May 19, 2021

EDIT changed the title to describe the problem discovered after investigation. Old title was "Invalid line number using "transpileOnly": true" - @cspotcode

Expected Behavior

Error stacktrace should show the exact, correct line number when using "transpileOnly".

Actual Behavior

It doesn't, the line number is off by around 8, although I imagine this changes.

Understandably, it is the same line or close to the line that appears in the compiled JS file, except that the file ends in .ts

(node:8464) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'ok' of undefined
    at Function.handle (src\api\routes\commands\POST index.ts:85:19)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at src\api\index.ts:305:21

The file can be seen here:
https://github.com/edqx/swagclan/blob/ee46636fcb42ce5c82c596861bab5b8ab963605b/src/api/routes/commands/POST%20index.ts#L77

And here is the compiled file:
https://gist.github.com/edqx/8bbc01d58ee0f759b09984920cd15bc3

I'm unable to create a minimal reproduction, currently I'm only looking for advice if this issue has been stumbled upon before, through searching the issues, I found #1055 but that issue has since been closed and did not fix my issue. I will, however, try to create one if really required, although I hope I have provided enough information as it is.

Specifications

ts-node v9.1.1
node v14.13.0
compiler v4.2.4

  • tsconfig.json, if you're using one:
{
  "compilerOptions": {
    "target": "ES2020",
    "module": "commonjs",
    "outDir": "./dist",
    "strict": true,
    "noImplicitAny": false,
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true,
    "paths": {
      "src/*": ["./src/*"]
    }
  },
  "ts-node": {
    "transpileOnly": true
  }
}

  • Operating system and version:
    Windows 10.0.19041 Build 19041
@cspotcode
Copy link
Collaborator

I haven't had a chance to investigate this issue or go through the issue with a fine-tooth comb, but thank you for including lots of detail; it will surely be useful. The following answer is incomplete but I hope is helpful.

I see you're on Windows. I've seen issues in the past where windows paths were either not handles correctly by us, or third-party components returned \ when we expected / and vice-versa. Sourcemaps are weird because I think they sometimes use Unix paths even on Windows. Sourcemaps are stored and retrieved by path in memory, so if we get the paths wrong, no sourcemap is used and line numbers are wrong.

I've also seen newline issues where we erroneously assumed Linux newlines and did some string manipulation wrong, which might be affecting how we manipulate the sourcemap comment to the end of the emitted .js string before passing it to node.

We have sourcemap tests, but it looks like none use transpileOnly. We should add that. The sourcemap tests are in https://github.com/TypeStrong/ts-node/blob/main/src/test/index.spec.ts and you can search for :100 since they all check for an error happening on line 100.

The code being executed by the tests is https://github.com/TypeStrong/ts-node/blob/main/tests/throw.ts It has a ton of whitespace at the top which will all be removed by compilation, which makes it easier to see what's going on. If the stack trace says the error happened on line 100, sourcemaps worked. If the line number is much lower, they likely failed entirely.

Unrelated, but I also noticed those test files do not use any type syntax, so if compilation was erroneously skipped entirely, the code would still execute. I should add a type annotation to ensure that compilation occurs.

@edqx
Copy link
Author

edqx commented May 20, 2021

Thanks for the detailed reply.

Ithink it's also important to note that the source map is not exactly on the line with the error. The source map says 83 while the stack trace shows 85. Maybe this indicates that the source map is being resolved by ts-node correctly?

The second one could make sense although I'm struggling to see how hard LF/CRLF is to manage in JavaScript, and where the issue might arise, but it's a possibility if you've encountered it before.

If you haven't already by later when I get the chance, I'll have a look at those tests and try to reproduce the issue in a minimal version since I think that would be pretty useful (... I doubt you want to go through setting up mongodb for my project).

@cspotcode
Copy link
Collaborator

I'm unable to create a minimal reproduction

Does this mean that when you tried to create a minimal reproduction, the problem went away? That would imply that there is a component of your project, removed in the process of trying to minimize the reproduction, that may be triggering the failure.

I see in the compiled .js that the error happens on line 83. This says to me that the sourcemaps are not being honored.

If you were to do the trick that we use in our tests, adding 100 or even 500 lines of whitespace to the beginning of the file, I bet you would see a much larger difference between the desired line number and what is reported in the stack trace. The stack trace would say line 83, but you would want it to say line 585. (due to the massive whitespace added to the file)

@cspotcode
Copy link
Collaborator

I see also that you have a space in the filename. Perhaps spaces in sourcemap paths are handled in a way we don't expect.

It is pretty easy to add additional debugging log statements to ts-node when you're trying to debug this.
Take a look at the contents of node_modules/ts-node/dist/index.js on lines 250-267
https://unpkg.com/browse/ts-node@9.1.1/dist/index.js

You can see that this is where we attempt to retrieve a sourcemap from memory. You can add console.log statements there and then run your project again.

This style of debugging is alien to a lot of developers, but in my experience it is pretty quick to do, and gives you lots of detailed, useful information.

@cspotcode
Copy link
Collaborator

I've added tests for --transpile-only sourcemaps with spaces in the filenames, and they're passing.

#1329

@cspotcode
Copy link
Collaborator

Are you sure that ts-node is compiling the file, or is it possible that node is running the precompiled .js file instead?

To test this, you can delete the compiled .js file from disk. Then run your program. If it runs correctly, this means ts-node has successfully read the .ts file, compiled it, and allowed node to run it. If it fails, this could indicate that node was running your precompiled .js file the whole time, meaning that ts-node was not given a chance to handle sourcemaps on your behalf.

@edqx
Copy link
Author

edqx commented May 22, 2021

Thanks for the reply again, I have only looked at the project earlier today and if I'm completely honest, I had completely forgotten about this issue as I didn't really encounter it aside from my manual "trying to break it" by adding a random line that would error. You're right that I failed to make my reproductions have the same error.

In regards to the trick that you use in your tests (adding the whitespace at the start of the file), you're right that the stacktrace remained at line 85 while the actual error should have occurred on line 344.

I'll start debugging through the package itself now and get back on my results.

As I began this while you posted your next 2 comments, I haven't looked into the sourcemap with the filenames since it seems as though that makes no difference, and I can confirm that ts-node is indeed compiling the correct file and running said compiled file.

@edqx
Copy link
Author

edqx commented May 22, 2021

I'm not really sure what I was supposed to see by adding a console.log here (last image), however I wonder if this was any use? It came up before the error was thrown.

The path variable seems to end at PO and not POST index.ts

image
image

@cspotcode
Copy link
Collaborator

cspotcode commented May 22, 2021

Interesting, can you copy-paste the full error there? EDIT I mean the full log message, errors, everything. Include as much as you can. The screenshot is truncated, and console.log typically adds a space between arguments. However, in your screenshot, I see POdata. I'm guessing there's more in that error that you may have missed and it'll be easier if I can see the entire thing.

@edqx
Copy link
Author

edqx commented May 22, 2021

I hadn't realised how large the logs were, sorry. I've posted them here https://gist.github.com/edqx/ab5005e8deb26a22a2a2f387e2ec62c7

Also I specifically meant the last one where it seems to say undefined for the second argument for console.log

@cspotcode
Copy link
Collaborator

Thanks, this is very helpful. This bit looks interesting:

//# sourceMappingURL=POdata:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoiQzovVXNlcnMvRWR3YXJkL

ts-node needs to manipulate the //# sourceMappingURL comments at the end of each compiled file. Perhaps, in this case, something went wrong with that manipulation.

For example, spaces in a URL are encoded as %20. The difference between %20 and is 2 characters, the same length as PO. I wonder if that's the culprit. Maybe we assumed a space, when it was actually %20, and so we didn't manipulate the string correctly.

I'm working on another task at the moment so I'll come back to this later. But I think you may have found the root cause.

@cspotcode
Copy link
Collaborator

In the 4.1.1 release
https://github.com/Microsoft/TypeScript/issues?utf8=%E2%9C%93&q=is%3Aissue+milestone%3A%22TypeScript+4.1.1%22+is%3Aclosed+

They implemented microsoft/TypeScript#40951

It percent-encodes the sourceMappingURL

@cspotcode cspotcode changed the title Invalid line number using "transpileOnly": true Sourcemap failing when filename has spaces or is otherwise affected by URL percent-encoding May 22, 2021
@edqx
Copy link
Author

edqx commented May 22, 2021

Why didn't this occur in your tests which had spaces in the filename?

@cspotcode cspotcode added this to the 10.0.0 milestone May 22, 2021
@cspotcode
Copy link
Collaborator

I figured it out. Final conclusion:

This is not related to --transpile-only; it happens with or without --transpile-only

This is already fixed on our main branch by #1160, even though the fix was not intentional. This may seem confusing but remember it is not related to transpile-only. #1160 changed the way we append sourceMappingURL, which also affected our non-transpile-only codepath.

Since the fix in #1160 was accidental, I've also implemented #1330 to make our internal logic understand percent-encoded sourceMappingURLs.

This fix will be released in v10 which I hope to release this weekend. In the meantime you can install directly from git: npm install TypeStrong/ts-node#main

I'm going to close this issue since all necessary work has been completed. Thank you again @edqx for your assistance locating the underlying issue.

@edqx
Copy link
Author

edqx commented May 22, 2021

Already waiting for v10 for the swc compatibility, thanks for the advice and information.

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

No branches or pull requests

2 participants