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

config does not play well with node-support-source-map #530

Open
1 task done
PavelPolyakov opened this issue Jan 15, 2019 · 10 comments
Open
1 task done

config does not play well with node-support-source-map #530

PavelPolyakov opened this issue Jan 15, 2019 · 10 comments

Comments

@PavelPolyakov
Copy link

I'm submitting a ...

  • bug report

What is the current behavior?

Currently, if you store your configs using ts extension and load them using config - source-map-support for node applications stops working. By not working I mean - there are no references to the original ts files in the stack trace of the errors.

I consider this functionality very important.

image

What is the expected behaviour?

They can work together and don't interfere each other (namely, config has no affect on how other modules work).

Expected behaviour:
image

Please tell us about your environment:

  • node-config version: v3
  • node-version: v10

Other information

Here is the repo where you can reproduce the issue:
https://github.com/PavelPolyakov/config-vs-node-source-map-support

@lorenwest
Copy link
Collaborator

I have a hard time categorizing this as a bug. There is no documented expectation of this library working well with this other library.

This sounds more like a feature request than a bug report. Anyone with .ts knowledge want to make a contribution for this issue?

@PavelPolyakov
Copy link
Author

@lorenwest
Thanks for the quick response. I see your point. Though I think that "good" library should be implemented in a way that it does not interfere with other libraries. That's what you, as developer, expect from any library, aren't you?

I don't say that config is not good, definitely it is and I appreciate your work and think it is great I use it often. However, I think that ts integration should be either not part of the config library or done differently.

Regards,

@lorenwest
Copy link
Collaborator

Agreed - it would be a good addition/improvement.

We have a few contributors that have added .ts support, and I'll mark this issue as a collaboration request.

@mrvisser
Copy link

I've updated the wiki (because it actually let me...) to indicate that .ts config files will mysteriously break source maps:

https://github.com/lorenwest/node-config/wiki/Configuration-Files#typescript---ts

This issue is devious enough that I think it warrants it.

@andrzej-woof
Copy link
Contributor

Hey, I've submitted PR #721 with a possible fix.

Tested locally on the demo repo https://github.com/PavelPolyakov/config-vs-node-source-map-support (after updating deps) and it seems to work on my end

@andrzej-woof
Copy link
Contributor

andrzej-woof commented Mar 3, 2023

Also I think it might improve or even fix #613 given that it should prevent unnecessary processing of files other than .ts


Quick test results on https://github.com/lavagri/nestjs-config-test (after updating config to latest)

config@3.3.9 from npm

[Nest] 62292   - 03/03/2023, 3:15:09 PM   ┌────────────────────────────────────────────────────────────┐
[Nest] 62292   - 03/03/2023, 3:15:09 PM   │    Starting APP MODULE: 2023-03-03T14:15:09.799Z        │ +1ms
[Nest] 62292   - 03/03/2023, 3:15:09 PM   └────────────────────────────────────────────────────────────┘ +0ms
[Nest] 62292   - 03/03/2023, 3:15:09 PM    * NestFactory import done +59ms
[Nest] 62292   - 03/03/2023, 3:15:09 PM    * Module import done +1ms
[Nest] 62292   - 03/03/2023, 3:15:09 PM    * AppController import done +1ms
[Nest] 62292   - 03/03/2023, 3:15:09 PM    * AppService import done +0ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM    * B import done +448ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM    * A import done +157ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM    * C import done +164ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM    * AppModule import done +1ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM   ┌────────────────────────────────────────────────────────────┐ +0ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM   │    END LOAD APP MODULE: 2023-03-03T14:15:10.631Z        │ +0ms
[Nest] 62292   - 03/03/2023, 3:15:10 PM   └────────────────────────────────────────────────────────────┘ +0ms

config@3.3.9 with fix

[Nest] 62163   - 03/03/2023, 3:14:46 PM   ┌────────────────────────────────────────────────────────────┐
[Nest] 62163   - 03/03/2023, 3:14:46 PM   │    Starting APP MODULE: 2023-03-03T14:14:46.461Z        │ +1ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM   └────────────────────────────────────────────────────────────┘ +0ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * NestFactory import done +58ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * Module import done +3ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * AppController import done +1ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * AppService import done +0ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * B import done +291ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * A import done +2ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * C import done +4ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM    * AppModule import done +1ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM   ┌────────────────────────────────────────────────────────────┐ +0ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM   │    END LOAD APP MODULE: 2023-03-03T14:14:46.822Z        │ +0ms
[Nest] 62163   - 03/03/2023, 3:14:46 PM   └────────────────────────────────────────────────────────────┘ +1ms

So there seems to be significant improvement for A, B, C modules import (297 ms vs 769 ms total)

@EvanTedesco
Copy link

I tried using @andrzej-woof's branch and I still had incorrect line reporting for failing Jest tests. The PR here seemed to fix the issue for me. require.extensions is deprecated and the require.extensions object does not even have the .ts key(It only has .js, .json, and .node as far as I can tell) so I am not sure the original intent of the removed code. In any case this seems to fix the incorrect line reporting issue on Jest test failures for my project so I figured I would test Cunningham's law and make a PR in hopes that someone more familiar with this library would save me a bit of code archeology.

Node v16.13.1
node-config: v3.3.9
ts-node: v10.2.0
ts-jest: v28.0.8

@andrzej-woof
Copy link
Contributor

Hi @EvanTedesco do you run your code via ts-node/ts-jest?
If so your issue is a slightly different one. The original problem in the repo concerns running tsc compiled code directly in node and transforming stack traces from js back to ts files with source maps.

I admit my PR won't solve your problem (as far as I understand it), but by removing this code I think you effectively disable loading any ts config files with compiled code (unsure if they're loaded at all even with ts-node.

Of course I might be wrong, just sharing thoughts here

@andrzej-woof
Copy link
Contributor

A bit more context, after some further investigation, regarding require.extensions being deprecated see discussion here - there seems to be on alternatives provided, and that property is still wildely used by other tools (babel for example)

As for running with ts-node it seems it works correctly on https://github.com/PavelPolyakov/config-vs-node-source-map-support when with ts-node ./src/server.ts (version v10.9.1), require.extensions is set, stack trace points to ts files. I have not checked ts-jest

@EvanTedesco
Copy link

Hi @EvanTedesco do you run your code via ts-node/ts-jest? If so your issue is a slightly different one. The original problem in the repo concerns running tsc compiled code directly in node and transforming stack traces from js back to ts files with source maps.

I admit my PR won't solve your problem (as far as I understand it), but by removing this code I think you effectively disable loading any ts config files with compiled code (unsure if they're loaded at all even with ts-node.

Of course I might be wrong, just sharing thoughts here

Yes I am running with ts-node/ts-jest. Thank you for the elaboration/clarification.

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

6 participants
@mrvisser @lorenwest @PavelPolyakov @EvanTedesco @andrzej-woof and others