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

Generate declaration files for js files if allowJs is set to true #1270

Closed
wants to merge 4 commits into from

Conversation

hediet
Copy link
Contributor

@hediet hediet commented Mar 12, 2021

Fixes #1260.

@hediet hediet changed the title Fixes #1260 by generating declaration files for js files if allowJs is set to true Generate declaration files for js files if allowJs is set to true Mar 12, 2021
@johnnyreilly
Copy link
Member

johnnyreilly commented Mar 16, 2021

Thanks for the PR! It looks like you've a failing test: declarationOutputAllowJs. On Windows you're generating:

      -      TS2507: Type 'typeof import("sub/dep")' is not a constructor function type.

But on Linux you're getting this:

      +      TS2507: Type 'typeof import("/declarationOutputAllowJs/sub/dep")' is not a constructor function type.

@hediet
Copy link
Contributor Author

hediet commented Mar 28, 2021

Thanks for the PR! It looks like you've a failing test

Do you have an idea what causes this? Seems to be unrelated of my change and an issue with the test itself, especially as it is OS dependent.

@johnnyreilly
Copy link
Member

johnnyreilly commented Mar 28, 2021

The nature of the difference between the two output is "sub/dep" and "/declarationOutputAllowJs/sub/dep". Which OS did you use when you wrote this test? It could be worth comparing your test to the existing declarationOutput one and see if you can spot a difference.

This may be surfacing an actual issue. If memory serves there have been historic issues with paths and declaration output. See:

#822

cc @JonWallsten

@hediet
Copy link
Contributor Author

hediet commented Mar 28, 2021

I'm a Windows user!

@johnnyreilly
Copy link
Member

What do you get if you generate the test output in the devcontainer?

@JonWallsten
Copy link
Contributor

I'll have a look today. Since the difference is between systems I guess the relative path is the culprit?

@johnnyreilly
Copy link
Member

Seems plausible - I wonder if the replace(/\\/g, '/') in afterCompile would be the fix?

@JonWallsten
Copy link
Contributor

Seems plausible - I wonder if the replace(/\\/g, '/') in afterCompile would be the fix?

My initial thought was that TS handles js files differently, because it already works with TS files. But since it works on one system and not the other I was also thinking about that fix we did! We should probably try that!

@johnnyreilly
Copy link
Member

Try all the things!

@johnnyreilly
Copy link
Member

I think this was done prior to the webpack 5 port. Could be worth regenerating comparison test output and see if the issue is resolved by the webpack 5 work

@as-zlynn-philipps
Copy link

Dang, this should be merged. Whatever happened with the failing test?

@johnnyreilly
Copy link
Member

I'm not sure - it all went quiet. If you'd like to pick this up and work on it I'll happily look at a PR!

@hediet
Copy link
Contributor Author

hediet commented Mar 18, 2022

Sorry, I moved on from the project I needed this for and then got very busy.

For this PR, the neccessary fix is in src/after-compile.ts. Everything else is about the tests.

@johnnyreilly
Copy link
Member

Awesome - I'm happy to assist with the test if someone else wants to pick this up

@mvilliger
Copy link
Contributor

#1260 has been fixed in PR 1483 and published with release 9.3.1.
Therefore, this PR is obsolete.

@hediet
Copy link
Contributor Author

hediet commented Jun 22, 2022

Awesome, thanks!

@hediet hediet closed this Jun 22, 2022
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.

No .d.ts Declaration Files for JS Files Are Emitted (allowJs: true)
5 participants