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

--ignore-not-supported, GenericRecords to object, resolve namespaces #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

tsconfig.json Outdated Show resolved Hide resolved
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

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.

None yet

3 participants