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

fix(tsConfig): path mapping by filing-cabinet #138

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jjangga0214
Copy link
Contributor

@jjangga0214 jjangga0214 commented Aug 16, 2021

The PR pending in filing-cabinet(dependents/node-filing-cabinet#100) should be merged and published before this PR to be merged.

I personally tested this PR against https://github.com/jjangga0214/ts-yarn-lerna-boilerplate, with filing-cabinet patched by dependents/node-filing-cabinet#100.

Closes #135

Related to:

Thanks!

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented May 29, 2022

@mrjoelkemp Hi!
Thank you for merging dependents/node-filing-cabinet#100.
Now, this transitive PR is ready to be merged as well!
Thanks!

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Jul 20, 2022

@mrjoelkemp Hi!
Would you mind if I ping you?
I wish this PR to be merged as I want to depend on node-dependency-tree from my new library, with this PR's feature.
Thanks a lot in advance!

@XhmikosR
Copy link
Member

XhmikosR commented May 5, 2023

Please rebase, drop any unneeded changes and make sure lint + tests pass.

@tianyingchun
Copy link

how progress of this issue?

@jjangga0214
Copy link
Contributor Author

@tianyingchun I will do it this week :)

@jjangga0214 jjangga0214 force-pushed the fix/135/by-filing-cabinet branch from c0c5fa5 to 48fd753 Compare May 27, 2023 08:15
@jjangga0214
Copy link
Contributor Author

@XhmikosR @tianyingchun Done it :)

@tianyingchun
Copy link

Waiting new release

@XhmikosR
Copy link
Member

XhmikosR commented May 27, 2023

A test is still needed.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented May 28, 2023

Should we add a test for cases like this as well?
I'd respect your decision, but I don't think so personally, IMHO.
This change is just a passing of a new single option to filing-cabinet.
Even if there's a bug or inconsistency, it's not what node-dependency-tree should find out and fix.
It's a dependency's job, by separation of concern and separated responsibility.
The test is already created on filing-cabinet's side, and should I recreate almost the same test case for the same purpose?
What do you think?
Thanks :)

@tianyingchun
Copy link

tianyingchun commented May 28, 2023

why we need two typescript config here?

tsConfig: config.tsConfig
tsConfigPath: config.tsConfigPath

Should we make tsConfig (string/object) works correct? instead of add new option tsConfigPath? Very difficult to understand

@jjangga0214
Copy link
Contributor Author

@tianyingchun I'd like to recommend reading this PR :)

@jjangga0214
Copy link
Contributor Author

@XhmikosR May I ping?

Thanks :)

@jjangga0214
Copy link
Contributor Author

@XhmikosR Hi? Are you super busy these days?

Thanks :)

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Nov 1, 2023

@XhmikosR Hello, dear maintainers!
This PR was ready-to-be-merged, but ignored for a long time and new commits came into the main branch, which is somewhat questionable. May I request more attention?

jjangga0214 added a commit to jjangga0214/haetae that referenced this pull request Nov 2, 2023
Through @jjangga0214/dependency-tree.
Refet to dependents/node-dependency-tree#138
@miluoshi
Copy link

Hi, can the be moved forward please? 🙏

@XhmikosR
Copy link
Member

XhmikosR commented Apr 3, 2024

I don't have time for dependents and I still think this needs a test case.

@miluoshi
Copy link

miluoshi commented Apr 3, 2024

@jjangga0214 can you write tests for the new option please?

@rhyek
Copy link

rhyek commented Jul 4, 2024

Really disappointed this isn't merged, yet. Kind of pointless use this lib with a TypeScript project as it is, since path mapping is very common.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(tsconfig): path mapping (module alias)
5 participants