Skip to content

Add types property to use generated declaration file rather than source #6

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

Merged
merged 3 commits into from
Nov 18, 2017

Conversation

Levertion
Copy link
Contributor

@Levertion Levertion commented Nov 16, 2017

Instead references the declaration file. This addresses an issue I've experienced which prevents me compiling my project as I have noImplicitReturns flag activated. (The problem function is in remove Node, but I can't actually see why there is an error (Error is Not all code paths return a value))

Instead references the file. This addresses an issue I've experienced which prevents me compiling my project as I have noImplicitReturns flag activated. (The problem function is in [remove Node](https://github.com/ShieldBattery/node-interval-tree/blob/master/index.ts#L386), but I can't actually see why there is an error (Error is Not all code paths return a value))
@2Pacalypse-
Copy link
Member

I'm not sure the correct solution here is to remove the inclusion of the source file when publishing. That was a conscious decision.

Instead, I think that we should just fix the error that you're getting. After a closer inspection, the remove function doesn't indeed return a value from all code paths. You can see it in this if/else if block: https://github.com/ShieldBattery/node-interval-tree/blob/master/index.ts#L404-L440

You could just add a return undefined below that block to satisfy the noImplicitReturns flag, even though code will never reach that point.

@Levertion
Copy link
Contributor Author

Is that to allow anyone who encounters an exception when running to see the source using the source map?

This has a disadvantage however if someone is using a more strict tsconfig.json than the module was originally created with, which then prevents their entire code from compiling properly. I think if you add just the types property in package.json it should work fine however (see microsoft/TypeScript#6964 (comment)). I'll add the source file back in then.

Also, I'm sorry, but I still am being very unobservant and am having trouble spotting where the unreturned value is 😕 . Could you please just fix that yourself - sorry!

@2Pacalypse-
Copy link
Member

2Pacalypse- commented Nov 17, 2017

I included the source file in my published files so it's possible to see the source code in the package without tracking it on github, which some people might want to do.

One thing I'm confused about your situation is why is your build process even failing due to this file? This file is not included anywhere and it's not used, so it definitely shouldn't be used by any build process, or linters. Can you check your tsconfig.json file and make sure that the node_modules folder is excluded? Or, if for some reason you can't exclude the whole node_modules folder, at least make sure to exclude the directory of node-interval-tree library. The main exported file of this library is already built, so it's not necessary to include it in your build process.

@Levertion
Copy link
Contributor Author

This issue is happening because I import something from node-interval-tree, and therefore typescript looks in node_modules for node-interval-tree, finds the folder. It then checks for a "types" parameter in package.json, then, when it doesn't find it, looks for index.ts in the package, starting from the package root. In order to use this file, it thinks it has to compile it, because it has not looked at index.d.ts in the lib folder.
This is ignoring the "exclude":["node_modules"] (Which I do have set), because it will try to compile any referenced file. See this explanation.

The fix is just to add a "types" attribute to the package.json, which is what I've done. This means it doesn't look at index.ts at all.

Also, I do understand adding the source code into the package, because as well as what you said, it allows the source map to work properly.

@Levertion Levertion changed the title Remove unneeded inclusion of source file for publishing to npm Add types property to use generated declaration file rather than source Nov 17, 2017
@2Pacalypse-
Copy link
Member

Ahh, thanks for the explanation. I'm not really that familiar with TypeScript, so this stuff is pretty confusing.

@2Pacalypse- 2Pacalypse- merged commit 363d6ba into ShieldBattery:master Nov 18, 2017
@2Pacalypse-
Copy link
Member

This is now merged and new version has been released. Thanks for your help!

@Levertion
Copy link
Contributor Author

No Problem. Happy to help!

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.

2 participants