ts-auto-guard
ts-auto-guard copied to clipboard
rhys-vdw
Declaration file is imported instead of actual class
If you were to use a class in your custom data type, it would be imported in the .ts file e.g. by:
import { Timestamp } from '@google-cloud/firestore';
This will result in non working import in the guard, where it will look like this:
import { Timestamp } from "../../node_modules/@google-cloud/firestore/types/firestore";
Two things are going wrong: 1: The import path should not be to the declaration file, but instead the package name as in the original file. 2: No relative path is needed for packages from node_modules
I really have tried to look through the ts-morph code, but havent been able to fix the problems.
Suggestion: I dont know how, but maybe ts.morph could identify, that the type is from a compiled declarations file and then use the original import statement instead of the relative file path from the SourceFile? Making an exception if sourcefile.isInNodeModules() is true might solve both problems, but i cannot find out how to get the original import statement to end up in the right place, since it only seems to be available on the parent sourcefile (via parentSourceFile.getImportDeclarations().forEach(declaration => declaration.getText())
@rhys-vdw could you help implement this exception?
Oh this is quite bad!
I've actually never used this to guard types from within node_modules. Does it always do a relative import?
To be honest I'm not really going to be in a position to help with this any time soon. You (or anyone reading this) are more than welcome to keep chipping away at it. I'm not really a ts-morph expert myself so it would be a lot of experimentation on my end too.
I wonder how we simulate node_modules in the test harness...
Oh this is quite bad!
I've actually never used this to guard types from within node_modules. Does it always do a relative import?
To be honest I'm not really going to be in a position to help with this any time soon. You (or anyone reading this) are more than welcome to keep chipping away at it. I'm not really a ts-morph expert myself so it would be a lot of experimentation on my end too.
I wonder how we simulate node_modules in the test harness...
Yes it does. So far i've coped with a post-build script that replaces the text. I'm surprised this issue has not been raised before. Not sure i'll get to the bottom of it. Let's see if anyone else can help.
I'm surprised this issue has not been raised before.
Main use case for this library is validating JSON data, where it's not possible to have class instances. I just threw the class check in for completeness. Usually if you've got code coming from JS (containing class instances) then you've already got a guarantee of type correctness from that code's types and runtime validation is not required.
Issue Status: 1. Open 2. Started 3. Submitted 4. Done
This issue now has a funding of 50.0 USD (49.93 USD @ $1.0/USD) attached to it.
- If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
- Want to chip in? Add your own contribution here.
- Questions? Checkout Gitcoin Help or the Gitcoin's Discord
- $5,856,761.66 more funded OSS Work available on the Gitcoin Issue Explorer
Issue Status: 1. Open 2. Started 3. Submitted 4. Done
Work has been started.
These users each claimed they can complete the work by 264 years, 11 months from now. Please review their action plans below:
1) tylerferrara has been approved to start work.
Investigating in your repository 👨💻
Learn more on the Gitcoin Issue Details page.
@wiegell's suggestion of checking if an import isInNodeModules() is a good place to start. I've created a proof-of-concept "fix" which resolves relative imports for all external packages found within a TypeScript file. Feel free to take a look here, as I create a dummy project where I replicate and resolve the issue.
If this approach is desirable, I can make sure it works for all types of imports and begin writing tests. Either way, I'm open to all feedback.
Feel free to take a look here, as I create a dummy project where I replicate and resolve the issue.
It's hard for me to assess the change because you do not have the project history for me to diff (kind of under the pump atm so I'm too busy to pull and diff the files)
Quick and dirty proposal sounds like it would work. Might be worth asking over at ts-morph if there's an easy way to get the correct import path from its type definition?
If you can't find a "proper" solution, or it's too difficult, I'm happy to accept the quick and dirty fix provided test coverage etc.
Gl @tylerferrara
@rhys-vdw yea the bot is from the bounty i set up :)
@tylerferrara Thanks for the very quick suggestion. I dont get the same output as you though:
Any ideas? Installed with yarn, but that shouldn't make a difference i suppose?
My apologies for not being explicit. Here are the steps to replicate the fix, from start to finish:
# Clone the project (fix is in master branch)
git clone [email protected]:tylerferrara/ts-auto-guard.git
cd ts-auto-guard
# Install ts-auto-guard dependencies
npm install
# Install dummy project dependencies
cd replicate
npm install
# Run the local build of ts-auto-guard on the dummy project
npx ts-node ../src/cli.ts --project . --export-all
This will produce the following file: ts-auto-guard/replicate/Person.guard.ts
/*
* Generated type guards for "Person.ts".
* WARNING: Do not manually change this file.
*/
import { Timestamp } from "@google-cloud/firestore";
import { Person } from "./Person";
export function isPerson(obj: any, _argumentName?: string): obj is Person {
return (
(obj !== null &&
typeof obj === "object" ||
typeof obj === "function") &&
typeof obj.name === "string" &&
(typeof obj.age === "undefined" ||
typeof obj.age === "number") &&
Array.isArray(obj.children) &&
obj.children.every((e: any) =>
isPerson(e) as boolean
) &&
obj.time instanceof Timestamp
)
}
@tylerferrara Yea i forgot to do another install in /replica :) Seems to work great - i cannot recreate the problem with interfaces either, since the guard does not need to import. I'm impressed with you quick solution - have you been using this repo before? I had a very had time navigating the ts-morph code when i tried myself :)
@rhys-vdw here is the diff
@tylerferrara since @rhys-vdw is fine with the "quick solution" please go ahead with the rest and the bounty is yours, thanks for helping out
@wiegell Thank you for the kind words.
I will say the ts-morph documentation could use some work by detailing implementation specifics, especially for the Other Information section. The only way of knowing this is to look at the source code for how they determine this.
As for this project, I've never worked with it before, but the general concept seems super useful! And your stats on npm seem to back that up. My experience with this bug wasn't too hard to jump into, but I would suggest more comments and or documentation as I'm sure your project will mature in the future. But it's a very neat and tidy repo overall. So congrats on building such a great tool 🎉
Cool. Well this has been novel. Glad you had no issues navigating that ~1000 line source file @tylerferrara. 😉
Um, not that I'm not happy with the solution and investment into the project, but is it not convention to open a PR with test coverage etc via this bounty system? Would be nice to have this solution available to all users.
Um, not that I'm not happy with the solution and investment into the project, but is it not convention to open a PR with test coverage etc via this bounty system? Would be nice to have this solution available to all users.
I think @tylerferrara will make one at some point? No rush though.
Great work @tylerferrara. There still are some other import syntaxes not covered. I'm sorry that i didnt participate in the PR, i wasn't notified by GH.
This is one:
This is another:
Tbh i'm not sure that this should be covered in this issue. Seems like its a problem not only of class imports from node_modules, but also classes internal to the project:
@rhys-vdw maybe a new issue for this?
It should be possible to work around in most cases unless you have to import a specific class name from two different sources.
(sorry I deleted a response there, you may have got an email notification for it, thought this pertained to another issue)
That sucks. I will reopen this issue. Given the simplicity of the change I wonder if this suggests an error on ts-morph.
@wiegell if you'd prefer to open a second issue you're welcome to close this one.
Issue Status: 1. Open 2. Started 3. Submitted 4. Done
Work for 50.0 USD (49.96 USD @ $1.0/USD) has been submitted by:
@wiegell please take a look at the submitted work:
- PR by @tylerferrara
- Learn more on the Gitcoin Issue Details page
- Want to chip in? Add your own contribution here.
- Questions? Checkout Gitcoin Help or the Gitcoin's Discord
- $6,286,097.86 more funded OSS Work available on the Gitcoin Issue Explorer
Issue Status: 1. Open 2. Started 3. Submitted 4. Done
The funding of 50.0 USD (49.96 USD @ $1.0/USD) attached to this issue has been approved & issued to @tylerferrara.
- Learn more on the Gitcoin Issue Details page
- Questions? Checkout Gitcoin Help or the Gitcoin's Discord
- $6,285,854.60 more funded OSS Work available on the Gitcoin Issue Explorer