Skip to content

Conversation

@Crusader4Christ
Copy link

Do not throw exception on non supported features.
GenericRecords transforms to object
Namespaces content are parsed

Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! Could you add tests for your changes? They would also make it easier to see what kinds of usage got fixed.

programPath.scope.rename(tsInterfaceCheckerIdentifier);
programPath.scope.rename(onceIdentifier);
programPath?.scope.rename(tsInterfaceCheckerIdentifier);
programPath?.scope.rename(onceIdentifier);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add these "optional chaining" operators in this file?
With the optional chaining operators, if programPath is somehow null | undefined (which it should never be), the calls to .rename() will be silently skipped, when we actually want an error instead (to let us know something went wrong)

Copy link
Author

@Crusader4Christ Crusader4Christ Sep 11, 2020

Choose a reason for hiding this comment

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

Sorry. I dont know why, but my Intellij idea tels me error is here. I'll reverted this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This hasn't changed yet with your updates. If you want to get rid of the TS errors you can use ! instead of ?, e.g.:

Suggested change
programPath?.scope.rename(onceIdentifier);
programPath!.scope.rename(onceIdentifier);

and that doesn't change the compiled code.

I'm not sure why the TS error doesn't come up when building.. I guess the build is not using "strict null checks" (--strictNullChecks), whereas your IDE is using strict null checks for some reason?

Do not throw exception on non supported features.
GenericRecords transforms to object
Namespaces content are parsed
@zenflow
Copy link
Contributor

zenflow commented Sep 12, 2020

@Crusader4Christ It seems the new test ("should compile namespace to runtime code") is also responsible for testing --ignore-not-supported. I think that --ignore-not-supported should have it's own test, like all the other flags. Also there's no updates regarding the new "GenericRecords to object feature". I think (if I can guess what it is) it can just be an addition to the sample.ts & sample-ti.ts fixtures.

So looking at the test you added, I can see how the "resolve namespaces" feature has been implemented.
I guess this lets us use ts-interface-builder/checker with types that are in namespaces, in basic scenarios, but what about:

  1. Types that depend on namespaced types. e.g. type IMyFoo: MyNamespace.IFoo | null.. will that work? (not sure)
  2. Types that have the same name as a namespaced type. Can I compile & use a type suite with IThing, MyNamespace.IThing & MyOtherNamespace.IThing? (with this implementation, no)

Just trying to look at this holistically, in terms of support for namespaces in general. Maybe this is a good stepping stone & these issues can be left for later, since I guess it would be a bit of work between this lib & ts-interface-checker.

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.

3 participants