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

Support composite indices in schema literal types #3609

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

nirvdrum
Copy link
Contributor

@nirvdrum nirvdrum commented Jan 9, 2022

This PR contains:

  • IMPROVED typings
  • A BUGFIX

Describe the problem you have without this PR

Following "Option A" for typing for providing a schema and generating the doc type in TypeScript from the Use RxDB with Typescript guide, I tried to add a composite index to the hero example with something like:

indexes: ['firstName', ['lastName', 'passportId']]

As far as I can tell, that syntax should be okay. I consulted the RxSchema docs to be sure. However, making that change causes a TypeScript compilation failure when using the RxJsonSchema type. Using the "hero" example from the aforementioned TypeScript guide:

const heroSchema: RxJsonSchema<HeroDocType> = heroSchemaLiteral;

I end up with the following compilation error:

Types of property 'indexes' are incompatible.
  Type 'readonly ["firstName", readonly ["lastName", "passportId"]]' is not assignable to type '(string | string[])[] | readonly (string | string[])[] | undefined'.

Todos

  • Tests
  • Documentation
  • Typings
  • Changelog

Since it's a compilation error rather than a logical error, please advise on how you would like to see this tested.

@pubkey
Copy link
Owner

pubkey commented Jan 9, 2022

Can you check why this breaks the CI?

@nirvdrum
Copy link
Contributor Author

nirvdrum commented Jan 9, 2022

Thanks. CI didn't run when I opened the PR because I'm a first time contributor. I ran all the tests, but it looks like the error is in building.

I've spent a little bit of time looking at it this morning and I'm not really sure how to resolve it at the moment. The core problem seems to be that Array.isArray has issues with ReadonlyArray. Various parts of RxDB use Array.isArray to narrow types and in those cases the readonly string[] is being treated a string because it fails the type guard. I'm not entirely sure how to resolve this. From client code, calling toTypedRxJsonSchema ends up deeply removing the readonly modifier, so there's no issue there. But for the library code, would it be acceptable to use an enhanced version of Array.isArray that also handles ReadonlyArray?

@pubkey
Copy link
Owner

pubkey commented Jan 9, 2022

Are you shure that it is about Array.isArray?
The first error is Error: [2] src/plugins/dev-mode/check-schema.ts(325,17): error TS2322: Type 'readonly (string | string[])[] | readonly (string | readonly string[])[]' is not assignable to type '(string | string[])[] | readonly (string | string[])[] | undefined'..
But we do not use isArray() at that line.
Same for the other errors that I have checked.

EDIT: Ok I see that might have sth to do with the isArray calls that happen before.

@pubkey
Copy link
Owner

pubkey commented Jan 9, 2022

Yes we can use a custom typed isArray method that fixes microsoft/TypeScript#17002

@nirvdrum
Copy link
Contributor Author

Adjusting Array.isArray as in the linked TypeScript issue presented a problem because there's code that assumes arrays are mutable (e.g., sortObject). Instead, I've tried to work around the issue with a new isMaybeReadonlyArray function along with a helper type, MaybeReadonly. I'm not married to the name or the approach. If you'd like to see something different, just let me know. Alternatively, if you don't want me to keep poking at RxDB's type definitions, feel free to close out.

Please kick off CI when you get a chance. I was hoping since you had previously approved it it would just run, but the workflow apparently still needs approval.

@pubkey
Copy link
Owner

pubkey commented Jan 10, 2022

I approved the CI again.

@nirvdrum
Copy link
Contributor Author

I fixed one of the two remaining failing jobs. I don't know how to run the "test:browser" one though. Running locally, the test never ends and just keeps printing:

t̓̒eͣ̾sͥͩt̀̇i̒͂̊n̐̍ ҉̬͚g̓͋ ҉̡̻ ͮ̇t̆ͧeͮ͂̓ś͂t͗̔͛íͦ͂n̆ͤg̒̔ ҉̢ ҉͓̪̜͔ ̓ͧ͌t̃̒eͩ̓͂s͒́̿tͦ̾ȋ̈n̂̈ǵ͊ ͐ͯt̉ͣ̚ ҉̛̕ẽͧ ҉̸͎̗̀s͌̈t̊ͦi̐͒n͊̀g̾̇̈ ҉̼̹̲ ͦ̊ ҉͎̣͇t̐ͨ̚̚è̈ ҉̢̦̥s̊͊t̾ͭ̋i͐̏n̏ͪgͪ͗͌ ҉͢ ҉ ҉͚̠̟ ͊̐t͌̄e̓ͤs͑̃t̂̓ͧî͛n̓́ġ̌ͩ ̾ͣͧ ҉͔̘tͭ̀ ҉̛̟eͯͨsͬͪtͧ͑i̊̐ń̍g͋̈
t̓ͫe͋ͮsͩͩtͫ̄i̿̊n̊͊gͧ̑̾ ̌ͧt̋̚ ҉̶͍è̃s̚ẗ̓̚i̐̑nͥ̌̎ ҉͚̗g̐̈͑́ ͣ ҉͇̀͟tͨ̏e͋̾́s̏́tͣ̿ ҉̥̱ī̄ ҉̖̻̯͕nͥ̈gͫ̓ ̅̇͗t̀̚ ҉̩eͨͫ̓s̐ͣtͥ̾i͆nͯ̈ ҉̷́g͒ͦ ҉̮͜ ͋͊tͩ̓eͧ͒sͬ̇t̅́ĩͤn̆ͦ ҉̸̘͔͈g̈͗ ̇̑t̂̎eͣ̅s̊͊ ҉̴̴ẗͩi̐̑n̊̌̀ ҉̳͈̬g͗͛ ͨ̚t͂̊̐eͦ̈͌s̈̿t̾̔iͨ̔n̆̀g̒̾ ҉̶̱
t̂ͮeͨͤͪs̃̐t̒͑ï̓ͯ ҉̰̣̮nͤ͆g͑̒ ͑̓t͗͆e͆̔sͯ͛t̔͛iͤ̏ ҉̕ ҉̼́n̅̾ ҉̕͜ğͭ ̓ͣ ҉̫̻t̆̏ ҉̸͢e̓̊s̄͂ͦtͦ͂i͌ͪn̎̆g̃ͪ̓ ̿ͧͭ̓t̔͋eͨ̄͑ ҉͜͡s̑̉t͊̚i̋̒ͭ ҉̳͈n͌ͨğ̀ ̎ͭt͛͆ ҉͇͖eͩ͐sͫ̓́t̊́ ҉̧̧iͧ̈ ҉ ҉̲͘nͥͯ͊g̋̐ ̃͐t͐͛eͫ̚s͌̏t̔̈̆ī͑nͮ̍gͤ̇ ҉̦̦̳ ̽t͑ͫe̋̈̆́s̍̄t̔̏ͬï̇n̆̄gͪ̚
tͮ̋́eͧ̚ ҉̢̮͡s͌ͪ̋t́̽ȉ̾̑n̑͗g̔̉ ҉̪͉̤̜ ͑ͪťͣ̌e̔̒ͤs̋ͪt̏̂͛î̅̓nͤ͑g͆͌ ̑̃t̊̊eͭ̃s͗ͩ́ ҉̰̼͓t͒͆ ҉̧̡͎̩̺i̿̾ ҉ ҉̺͖͠n͑̚g͌͒ ̍̋t͆ͭeͦͧ ҉̦̺s̽ͦtͣͪ ҉̫͠ĩ̂nͧ̈g̏ͫ ̈̂ṫ̀e͗̍͆s̽ͨẗ͐i̽̆n̄̏ͣgͦ͛̃ ̓̓ͤ ҉̦͓tͤ͌e̿̑s̈ͨ ҉͘ ҉͕̘͔t̽̚ȉ̾ ҉̡͡n̋ͬg͂̈ ̿ṫ̍ ҉̦̻ȇ̃s̓̐ͥtͧ̚i͐̑n̎ͦg̓̋
tͫ̈eͨ̇ ҉̧̤͙sͯ̍tͤ͗i̔͗n̐ͦgͩͥ ͋ͫtͯ̃ē̒s̅ͥt̎ͪiͬ͐n̒̂gͨ ͋ͬt́̿e̓̑s͂̈́tͬͤi̔̅nͥͣǵ͂ ͩ͗t͐̓̏e̿͛sͣ͋̌tͩ̀i͆̇̑n͋̿g̉̆ ̾ͩ̋tͤͯěͨs͒ͩ͐t̑ͦî̀ń̓ ҉ ҉̮̞gͯ̉ ͬͭtͦ̍͆eͩ̔s̔ͭ ҉̡͙ṫ͌i̓ͨ̾n̉͌g͋͋̍ ͩ͂̚t̿̓̓ë͒s̏̔ ҉̀́t͊̌i̓̎ͫ ҉̗̦̕n͋ͭgͣ̈ ҉ ҉͝ ҉̪͉̤
t͂̂e͗͒͐sͨ̓tͭ̾ ҉͠͞iͨͤ̓n̉̓g͐̓ ͋͊tͬ̐e͂ͫͪsͨ̊t͛ͪi̅̏n͋͐ ҉̳̯̫g̊͋ ͤẗͦ̒ëͬ̿s̆̊tͯ̿i̅̇n͆́̌̿g͐̅͂ ҉͔̘ ̃͑t͒̄e͌̍s̎͑t̆̃i̓̉͗ṅ͒g̈̌ ̐͆́tͬ̏ ҉̝̫ĕͯs͐̈ ҉̥͙tͪͩiͩ̈n̒͒g̈̓̂ ̐̈̄tͧ͗ ҉̡̯̬eͧ͒ ҉̸̘̹s̆̉́t͋͂͒ī́n̂͊̊g̔̅ ̔̚t͆ͨe̋͌s̓̍t̔̒i̽̚n̊̐̏g͌̉
t̏̐é̍ͣsͣͤt̉̇ï̈ͪnͧ̅g̔ͧ ̏͊ ҉͔̘̖̭t̑ͯ̄eͤ̋s̒̈t͑ͯi̔̃n̓͆g͛͊ ͑ͤt̊̓e̾ͨ͋s̾ẗ̅i͌̎n̋͌g̀ͪ̋ ̔̚t̔̑ ҉̵͕͔͓ͅȇ̄s̾̄ ҉͙͢͝t̆̋i͐ͮñͭgͩ́ ҉̬̘͇̼ ͋͋ͬtͧͮe̍ͥs̎͂ͦt͋͂iͥ̾n̐ͭg̓ͫ ͑ͩ̀ ҉̸͟t͛͒e̿̚s̑̍t͂̿̅i̎̈ ҉̘̖͟n̈̌g͑͊̄ ̆̀t̆ͯĕͥsͫ͛ͪ̏ ҉͘͞ṫ̂ȉ̃nͭ̆̉ğ̓
tͭͦe̐ͩsͧ̅t͋̈i͐ͫn̆̆g͒ͫ ̆̈̏tͦͥe͋͆ ҉̢ ҉̯̀s̃̉ ҉̰͉̝̘͢t̓͆ͧi͐̌̈n̂̍g͊͛ ̀̾ ҉̲̀tͪͧe͋̀ͬs̒̿t̒̈̑i̇́n̔͒gͭͯ ̃̀tͪ̉eͪ̚sͥ͋ͭt́ͬi̿̍ ҉͖̰̘̞n̐̓ĝ̎ ҉ ҉͘͟ ̋ͤ̎̌t̾̓ ҉̸̫͜eͯ̀sͯ̈ͨ̚t͑ͫ ҉̩̝̗͝iͨ͊n̋̉̍g̓̈̚ ͋͒t͆̿e͑̆s̄ͩt́ͯi̚ ҉̡̫̜̫̩n̑ͯgͬ̑̓ ҉̟̖ ͧ͗t̎ͩ ҉̡̡e͆ͤ̊sͩ̈̿t̓̈͂iͧ̋n͂̑ ҉̷̢̼g͐̈
tͥͪ ҉̕͢eͥ̌sͣͥͬt̉̓i̓ͣn͂ͤͣg̋̈ ҉ ҉̀͜ ́̈t̔̑ẻ̚ ҉̯͜s͌̽ṫ͛î̎nͯ̔g̋͗ ҉̙́ ͮͨͤtͥ̑ ҉̀ ҉͕͠eͥ̓s̾̏͗tͣͬiͭ̈́n̋ͣgͦ̚ ͨ̚ ҉̡̟̬t́ͯe̒̌ͭs̽̉̑t͆́i̾̃ ҉̰̖̞n̔ͥ ҉̛͝ḡ̽͂ ̑̓ẗͬeͤ̓ ҉̡̕s̎ͥ͂t͛ͭ̽i͗ͣn̓͑̈ǧͥ̽ ̅̈t̎̓e͋͒š̿t̋͛ͩí̃n̾̉ğͫ ҉̴̭͘ ͗ͧt̄ͫeͤͧs͋̈̒tͩ́i̿̓n̒̔ ҉͎̝̘̠g͗ͨ
t͗͋ě̌s̿̅t̑̎ͨï̒n̉ͧ ҉̛̜̣g̊̓ ̋ͫt͋ͪͪͨeͬ͆̈s̍̏ ҉̰̮t̐ͬi͆ͭͭnͨ́g̈ͫ̓ ͆̽t͂̃eͦ̋sͯ̽̾tͩ͑íͥnͫ̚g͆̂ ͋̔t͒̈e͑̆̓ ҉̤̀s̑̈t͑̓iͬ͐n͐ͩ̉ḡ̒ ͂̑̒t̓̾́eͦ͂̈s̈ͭtͧ̚ ҉̢͠i͑ͣ ҉̷̩̺͚ͅn͌̓gͯ̍ ̔̾ ҉͈ṫ̈ȇͤs̈ͭẗ̚i̇ͬ̈ ҉̵̺̻n̏g̐̽ ̆t́ͦe͐̂s͋̉tͨ͂ ҉ ҉̬̻̠̲̳ǐͦn̽͋g͐͆
ť̍eͫ̚sͭͩtͬ̚ ҉̯̠̫̀iͪ̅nͮ́̔g̐̅ ͐͊̓ ҉ ҉̫͚̣̲̜t͆ͪe͊̽s͗̒t̑ͨiͩ̋n̚̚ ҉̡ ҉̟g̈͊ ͂̀tͨ̿e͌̆s̽͂tͯ͐ ҉̧͕͟i̇̇ͣ̏ͥn̄̓gͩ̄ ̍͑̉t̓͗e͋ͭ̓sͯͥtͬ̏ȉnͯ́gͧ́ ̿͒t̄̄eͧ̎͑s̾̚tͥ͗i̾͋ ҉̕͞nͥͩġͧ ͨ̚tͪ̋̿e̊̾ś͒tͩ͛î̅ ҉͔̘n̒ͬgͯ͋̆ ̆̾tͧ̇ě̑s̋̓ ҉̼̞t̀̈iͯ͋nͬ ҉̜ͅg̅ͪ
ťͪĕ̈s͌̽t̿̈̽ỉ̔ ҉ ҉̤͔̲n̿ͥ͌g̾̅ͫ ̈ͣtͭ̉e̎͐sͣ̏͆tͥ̍i̎̾nͯ̋ͪ ҉ ҉̯̜gͥ͐ ̈ͬt̅̌ẻ̂s͋͑t̄ͩiͬ̓n̓̚gͮͩ ͣ̈t̽̇eͭ ҉͢͝ś̿̀t̆͌͑i̾̋̽nͣ͌ͫg͊̆ ͨͩͮt̊̈̔eͤ̈s̅ͤ̚t̏̂ͨi͋̓nͬͨg̏ͦ̀ ҉̨̱͝ ͌̓t̍ͫ̅e͛̈sͧ̎ ҉͟ ҉̱̖tͨ̓i͛̃͊n̾͋g̓̾ ͌ͪͥt͛͒ ҉̵͎͝ě͊śͪ̈tͫ̃ ҉͙̲̯i̿͐ňͨ ҉̠̠̲g̿ͪ
t̅̉e͗͂s͊̓t̐̌ ҉͙͞i̎͆ ҉̴̥̟n̋̓g͛̈̈ ̋̀ ҉̦͡ͅt̂̔ ҉̀͢ë̇s͛̓t̓ͩi͑̒n͗̉g̿ͪ ҉̙́͜ ̈̈t̎̓eͬ̍s͛̌tͥ̈i̇͒n̿̒g̃̇ ̎͒t͆̊̑e͂ͥs͂͛tͤ̈̇i͗̂n̉ͣg̉̍ ̽̋t̍̂e͊̿̏sͫ͐ͧt̿̍iͭ̏nͣ̓g͊̒ ̌ͨt́ͫ ҉̀͡ȇͯ̔ͪṡ̉t͊̅̎iͪ͊ ҉͇͕̗n͋͌g̋̊ ̈̚t͊͒ė̾s̆̈t̑ͭiͫͥn̎̇gͤ̔͐ ҉̦̻
tͯ͐̇eͧͧŝͦt̀̋īͯͯn̅̄g̉́ ͨͯt͊ͥeͮ̓s͊t̎̂̉ïͥn̍ͣ ҉̵ ҉̙̪̮ͅg͗̄ ̄̈t̓̆eͩ̿ ҉͈͖̱̩͝s̽ͪt̍ͣ ҉̸̕iͪ̎n̎ͮg̑̔ ͛ͭ̍t̿̓ẻ͋̚sͩͫt̀̋ ҉̛̘̦iͨ̚n͑ͭgͨ̓ ͨ̎̌t̒̓̃e͒ͦsͪͣ̋t̐ͮiͣ̄n͂̉͂g̾͌ ͣ̓t̋̋̋ͤeͬ̾s̒͌t̓̓ͭi͌ͩ ҉̸̨̟n̆gͫ̚ ͨͥ̄ ҉̣t͋̆̾͒e͋̚ṥtͬͦi̽ͥn̂̚gͯ̄̎
tͧͥe̊͗s̅ͬ̀ ҉̸̢t̓͆́iͤ͂nͩ̂gͪ͗͌ ̂̅͊t̃̚e͒̽ ҉̘̫sͧ̐ ҉̹̝t̓ͤi͐͌ǹ̒g͐͌ ͫ̇ ҉̷̧t͂͋eͯ͆̊sͥͨt͗̽iͤ͗ͧ ҉̶̵n͆̉g̿̾ ̇̏ ҉͈͔t̐̑e͗ͫs̔ͭt̓̑i̽̈n̈ͤ͐g̀ͯ ̌ͧtͣ̽ ҉̡͙e͌͆s̒̐t͌̽i̐̽ ҉̮̪̩̻n̉ͥg͑͗ ҉̭͜ ̓̋̈tͩ̿e̔ͬs͑͗t͆ͬ̊iͣ̚n̓̈͐gͨ͑ ҉ ҉́́ ̽ͣt͗ͨ̒e̊ͨ̀s͂̿́tͤ̌̎i͌ͥnͦ̃gͣ̏̾
t̊̚e̔ͥ̈s̈̚t̂́i̇̓ń̚ǵ̀ ͪͥt͒̈e͆̈ṡͮt̄̆i̽̌ ҉̵̬̠ń̆g̑ͩ ̑̓t̓̂e̓̎ ҉̪͔̩sͭ̐t́ͯ ҉̨̞i͛͒nͭ̍g͌̈ ̓ͧt͒͐eͥ̈ṡ̅t͐̀ĩ͊n̈ͧğ̔ ͯ͆̃ ҉̵̘tͫ͒e̐̊s̽ͭt̓̓i͂̈nͮ̾ͥg͊͗͂ ͛̃t̋ͮe̿ͮ ҉̵̨͈s̄ͪ̊t̆̃i͗̎nͫ̏ġ́ ̃͑t̂̎ȅ̍ŝ͗̊̇t͊͂i͛̊ ҉̴̠̀ň͆̂̎g͊̇
t̐ͣ̈e̒ ҉̵̠s̅ͫt̀ͥï͌n̈̐ ҉̡̺̕g̋̚ ̊̏t͂̓ë̔ͫś̿̀ ҉̶̸̮t̅̅͂i͊̓nͦͧgͧ̑ ̾̆t̎̌̑e͆̓ṡ͗ͭt̂̋ͨî̋n̔͐g̽ͤͮ ͊̏ ҉̸̻̳̫ṫ̓ê̌sͥͭ̄͋t̍ͪi̿͌̂n͋̾͂g͊̓̈ ̐̌t̐̄ ҉̲͢͠e͑ͮ̾s͂̓tͩͮ ҉̘̺͘ȋ͌ ҉͎͞n̍̇̚gͨͩ ҉̩͢ ͭͩtͭ̐ͩé̚sͬͪt̓͗̍ ҉̲̥̯̟íňͧg͌͗ͫ ̑̑͆tͪ̐e̋̐s̊̄t́̆̇ ҉̘͡i̍̚ ҉͘͞ ҉̙̖͈͟n̈̚g̃̇ ҉͕̭̝̳
t͗̿e̐ͣsͫ̊̍tͮ̓í̈n̓ͫ͛ ҉̥͢g̉̔ ҉̗̬̠ ̋ͭ́t̉̆é͊s͑̿ṫ̂i͛̈ ҉̵̧n̆ͦg̃̃ ͋̔t͛̓e͒ͭs̓̚t̎̄ ҉͖̮ȋ̇n͆̇ġ́ ͦͧtͪ̏e̿͒s̈́̈tͮͤiͣ͒nͦͥg̋̊ͪ ̅ͭt̑ͮe̾͛sͬ̉ ҉̕͢t̿ͪỉ̊nͧ̐g̾͌̊ ͪ̚tͯ̈̓ȅͧs̃ͭ ҉̵̠̭̝͍t͊͗ĭ̐́ ҉̷̫̥nͧͣğ́͌̐ ̌̀ṫ͗eͥ͆s̾̈tͬ͑i̒̆n͗̾ ҉͔̘̖gͥ̐̚
t̐ͣeͥ ҉̴̨̞̀sͫ͒t͌ͧi͗͑nͭ̈͌gͨͥ ̐͊t̑ͣeͫ̚s͂̇t̓́ ҉̥͙iͮ͒nͥ̈g̍ͦ ̐ͧtͦ͒e̅͗sͥ͋ẗ̓̚i͛̾n̒͆ḡ̈ ҉͈̬ ̾̿̚t̒͆e̎ͬs̃̓t̾̚i͊́n͌g̈ͫ̎ ̈ͧ ҉̱͇̟͔t̓ͤ̂ĕͫsͯ͆t̑̌i͊̓n̏͆g͆̍ ҉̹͞ ͒̊tͯ̃ê͗sͬ͌t̽̌i͆̿nͦ̋g͊̌ ̑̓t̒̌éͯͦs̈̆t̋ͧi͐͌nͧ̍ ҉̛̙̜̙ǵͫ͂

I see the same thing running with the master branch. I'm guessing it's some environment thing, but I don't know where to even start looking into that.

@pubkey
Copy link
Owner

pubkey commented Jan 11, 2022

Master seems to be green again. Can you rebase?

@nirvdrum
Copy link
Contributor Author

Will do. I figured out the messed up output was due Karma installing the trashed version of colors. There's a PR to lock that down in Karma. I'll drop the commit where I locked that down if it's no longer needed.

@nirvdrum
Copy link
Contributor Author

@pubkey Rebased and pushed. I did not commit the dist/ files. I couldn't tell if that was something done manually or as part of a workflow. If it should be done manually, just let me know.

@pubkey pubkey merged commit 08db75e into pubkey:master Jan 11, 2022
pubkey added a commit that referenced this pull request Jan 11, 2022
@pubkey
Copy link
Owner

pubkey commented Jan 11, 2022

Merged, thank you for your contribution.

@nirvdrum
Copy link
Contributor Author

Thanks for being patient while that got sorted out. Hopefully future contributions go smoother. Do you think you'll cut a release soon-ish? I can set RxDB up as a git dependency, so not a big deal either way.

@nirvdrum nirvdrum deleted the patch-1 branch January 11, 2022 21:54
@pubkey
Copy link
Owner

pubkey commented Jan 11, 2022

I will do tomorrow

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