Skip to content

Various bug fixes and improvements. #17

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

Closed
wants to merge 10 commits into from

Conversation

bitonic
Copy link
Contributor

@bitonic bitonic commented Apr 8, 2020

I suggest to review commit-by-commit, they do one thing each and they should explain well what the fix is.

There are no tests yet -- I hope to get around to it soon.


This change is Reviewable

bitonic added 5 commits April 8, 2020 15:43
...and exclude grandparents. The immediate parents are needed to make
sure that we don't miss them when computing closures. The grandparents
are not needed -- we'll get them through the immediate parents.
We just return the type of the parent. The rest will be taken care
by `getParentTypes`.
Before this change, the following type

    data Test = TestBlah {x :: Int, y :: Bool}

when configured with `tagSingleConstructors = True` would have generated

    getTypeScriptType (Proxy @test) ==> "Test"

    formatTSDeclarations (getTypeScriptDeclarations (Proxy @test)) ==>
        interface TestBlah {x: Int, y: Bool}

That is, there would be a dangling references to a non-existent
interface `Test`.
This allows to have unions as keys, which often happens when encoding
variants made out of nullary constructors.
Copy link
Contributor

@thomasjm thomasjm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bitonic ! Most of these changes look great.

However, I added a few tests to exercise the Maybe stuff better and there are some failures. I stuck a commit onto your branch here to demonstrate: https://github.com/codedownio/aeson-typescript/tree/francesco

These test failures aren't exactly your fault, it's more due to how this library hasn't properly handled omitNothingFields in the past and has always encoded Maybe using question marks even when T | null would be a better choice.

Any chance you'd be open to fixing those tests in this PR?

@@ -93,12 +93,14 @@ data FormattingOptions = FormattingOptions
-- ^ Function applied to generated interface names
, typeNameModifier :: String -> String
-- ^ Function applied to generated type names
, exportTypes :: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a documentation string here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also curious about the need for this -- I always put my types in .d.ts files, is there some reason you want to put them in an ES6 module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generate automatically not only the types, but also automatically generated code that builds a data structure that holds fetch invocations with the right request and response type, through https://github.com/bitonic/solga/tree/francesco/solga-typescript . I store the generated types alongside this definition, and therefore I need the exports.

@bitonic
Copy link
Contributor Author

bitonic commented Apr 15, 2020

@thomasjm thanks, I'll take a look at those soon.

@thomasjm
Copy link
Contributor

The latest commits look good @bitonic . I'd love to get this merged and get a new release out with these improvements. Are you planning to get the tests passing? Thanks!

@thomasjm
Copy link
Contributor

thomasjm commented Aug 5, 2021

Working on integrating the remaining changes from this PR:

  • 92fdfba 2020-07-08 | Add instance for Word8 (bitonic/francesco) [Francesco Mazzoli]
  • b61824c 2020-05-22 | add parent types / generic instances for unary records [Francesco Mazzoli]
  • f6e51e4 2020-04-15 | cabal file update [Francesco Mazzoli]
  • a751b8b 2020-04-15 | Allow to access TypeScript types from outside [Francesco Mazzoli]
  • 13822f0 2020-04-08 | allow to export types easily [Francesco Mazzoli]
  • 2a86676 2020-04-08 | use mapped types to represent maps [Francesco Mazzoli]
  • 56f7de2 2020-04-08 | add some missing container instances [Francesco Mazzoli]
  • 7645e67 2020-04-08 | when the type refer to a single interface, refer to it correctly [Francesco Mazzoli]
    • I tried this and it seems to be fixed now, must have been addressed in a later refactor
  • 6ffc83e 2020-04-08 | honor unwrapUnaryRecords for unary records [Francesco Mazzoli]
  • cd8e46a 2020-04-08 | include immediate parents for containers... [Francesco Mazzoli]

@thomasjm
Copy link
Contributor

thomasjm commented Aug 6, 2021

Okay, I think that's everything! I'll get a release out with all this stuff soon. Thanks again.

@thomasjm thomasjm closed this Aug 6, 2021
nh2 added a commit to nh2/aeson-typescript that referenced this pull request Oct 4, 2021
No point having a default when the user cannot access to override it!

This was forgotten in PR codedownio#22 when extracting it from PR codedownio#17.
nh2 added a commit to nh2/solga that referenced this pull request Oct 4, 2021
…ixes.

Our fixes: codedownio/aeson-typescript#17

The one that wasn't taken verbatim, which this commit adapts to:

Upstremed in: codedownio/aeson-typescript#22

We still have to use our fork to fix `defaultFormattingOptions` not being
exported:

codedownio/aeson-typescript#27
thomasjm pushed a commit that referenced this pull request Oct 4, 2021
No point having a default when the user cannot access to override it!

This was forgotten in PR #22 when extracting it from PR #17.
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