-
Notifications
You must be signed in to change notification settings - Fork 19
fix: Don't mix generated declaration files with ts source files. #91
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: Don't mix generated declaration files with ts source files. #91
Conversation
package.json
Outdated
], | ||
"scripts": { | ||
"test": "npm run test:js", | ||
"test": "yarn run test:js", | ||
"test:js": "mocha --require ts-node/register tests/*-test.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self, please remove ts-node/registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed with: #92
742c7e9
to
77bc99e
Compare
Good catch!
Good catch!
In general, I try to support node versions until they are EOL'd, node 11 is get's EOL'd in June, at which point we can make this change, but it will be a considered a breaking change and i'll do a major bump. |
Interesting, if you can share the scenario I would love to figure out what was going on. When I compare the |
@stefanpenner Here's a simple project that reproduces the issue. https://github.com/chriseppstein/reproduce-composite-build-issue This feels like it may be a bug in typescript so I'm going to file a bug against it to get their feedback on it.
I don't think that's true. For example: The
This is different from before because the |
@stefanpenner FYI the typescript team considers this a bug and already has a PR for the fix: microsoft/TypeScript#31191 |
TypeScript seems to get confused when a .d.ts and a .ts file of the same basename are in the same directory. This causes a consuming project to think it needs to compile the *.ts files that are imported from the index.d.ts file. Also did some other maintainance while I was here: * use yarn instead of npm for script execution. * enable sourcemaps so consuming projects will see the right source when navigating code and so debugging sessions can navigate the typescript code.
77bc99e
to
49b6a3a
Compare
@stefanpenner with the current directory structure of having I can preserve the location of the output files and have a single tsconfig file if I move |
FYI broccoli-dependency-funnel does a deep require into fs-tree-diff. IMO that means that |
@chriseppstein thank you for looking into this and opening discussion with typescript folks. As they believe this is a bug, and have already submitted a fix, does this project need to make any changes?
This would be a breaking change. |
Probably not. There are two work-arounds:
|
I'm trying to understand the intention of Unfortunately, I haven't really found many good resources, I'm going only on:
And the above linked bug. Obviously I'm not TypeScript expert, so if I have misunderstood something please feel to correct me. But as is, I believe this project seems to behaving correctly and should not work-around a bug in typescript. |
TypeScript seems to get confused when a .d.ts and a .ts file of the same basename are in the same directory. This causes a consuming project to think it needs to compile the *.ts files that are imported from the index.d.ts file.
Also did some other maintainance while I was here:
Update engine support to exlude node 11 now that 12 is released.