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: Don't mix generated declaration files with ts source files. #91

Closed

Conversation

chriseppstein
Copy link
Contributor

@chriseppstein chriseppstein commented Apr 26, 2019

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.
  • Update engine support to exlude node 11 now that 12 is released.

package.json Outdated Show resolved Hide resolved
package.json Outdated
],
"scripts": {
"test": "npm run test:js",
"test": "yarn run test:js",
"test:js": "mocha --require ts-node/register tests/*-test.ts",
Copy link
Owner

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed with: #92

@stefanpenner
Copy link
Owner

use yarn instead of npm for script execution.

Good catch!

enable sourcemaps so consuming projects will see the right source when navigating code and so debugging sessions can navigate the typescript code.

Good catch!

Update engine support to exclude node 11 now that 12 is released.

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.

@stefanpenner
Copy link
Owner

stefanpenner commented Apr 29, 2019

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.

Interesting, if you can share the scenario I would love to figure out what was going on.

When I compare the src this PR produces, vs the lib contained within published tgz of the current released version they appear to both contain the same files. Specifically, both contain the same basename.{js,d.ts,ts} configuration, that seems to be giving you issues.

@chriseppstein
Copy link
Contributor Author

@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.

When I compare the src this PR produces, vs the lib contained within published tgz of the current released version they appear to both contain the same files. Specifically, both contain the same basename.{js,d.ts,ts} configuration, that seems to be giving you issues.

I don't think that's true. For example: The src/index.ts file stays in the src directory. Into the lib directory it generates 4 files:

index.d.ts
index.d.ts.map
index.js
index.js.map

This is different from before because the index.ts file isn't in the same directory as index.d.ts.

@chriseppstein
Copy link
Contributor Author

@chriseppstein
Copy link
Contributor Author

@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.
@chriseppstein
Copy link
Contributor Author

chriseppstein commented May 1, 2019

@stefanpenner with the current directory structure of having src next to test I don't think there's a way to preserve the lib directory output and have a single tsconfig.json file. Usually with this structure the output is to a single directory dist that contains lib and test output files. But if I do that, an existing require('fs-tree-diff/lib/entry') will fail after this code is released because the file will have moved. I'm not sure if you consider that a breaking change or if it is a violation of your public api boundary.

I can preserve the location of the output files and have a single tsconfig file if I move test to be a subdirectory of src. Or I can have two tsconfig files one for test and one for publish.

@chriseppstein
Copy link
Contributor Author

chriseppstein commented May 1, 2019

FYI broccoli-dependency-funnel does a deep require into fs-tree-diff.

https://github.com/ember-engines/broccoli-dependency-funnel/blob/master/src/index.js#L7

IMO that means that Entry should be exposed via the primary export of the module as part of the public api. It also limits what we can do with the output structure.

@stefanpenner
Copy link
Owner

@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?

But if I do that, an existing require('fs-tree-diff/lib/entry') will fail after this code is released because the file will have moved. I'm not sure if you consider that a breaking change or if it is a violation of your public api boundary.

This would be a breaking change.

@chriseppstein
Copy link
Contributor Author

does this project need to make any changes?

Probably not. There are two work-arounds:

  • disable the composite setting in the consuming project. Projects that don't set composite: true cannot be used as a project reference. For me, this is not a big deal, but for other projects it might be.
  • Upgrade to typescript@next, which is considered unstable and may have other issues.

@stefanpenner
Copy link
Owner

stefanpenner commented May 4, 2019

I'm trying to understand the intention of composite, after some research and experimenting it seems like it is intended to be used for modules contained within a mono-repo style setup. It does not seem like something one would expect to work across arbitrary out-of-repo modules provided by node_modules. Or at the least, one would not expect it to force coupling with those out-of-repo modules.

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.

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.

None yet

2 participants