Page MenuHomePhabricator

[lib] Just cut/paste values into `minimallyEncoded` validators
ClosedPublic

Authored by atul on Nov 9 2023, 1:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 31, 2:25 AM
Unknown Object (File)
Oct 15 2024, 1:19 AM
Unknown Object (File)
Oct 15 2024, 1:19 AM
Unknown Object (File)
Oct 15 2024, 1:19 AM
Unknown Object (File)
Oct 15 2024, 1:14 AM
Unknown Object (File)
Oct 10 2024, 5:21 PM
Unknown Object (File)
Oct 6 2024, 1:29 AM
Unknown Object (File)
Sep 14 2024, 5:09 PM
Subscribers

Details

Summary

Changes in D9808 did not resolve Jest issue.

Spent way too much time trying to get this to work, and the core issue is that we're spreading in base validators...which is such a minor thing so I'm just going to cut/paste them for now to unblock.


Depends on D9808

Test Plan

Tests continue to pass, Jest issues are no longer a problem.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Nov 9 2023, 1:20 PM
ashoat requested changes to this revision.Nov 9 2023, 1:37 PM

This is super hacky – can you spend some more time explaining the error you're seeing and what you've tried?

PS – it's always best to document each step in an investigation live on Linear... saves you time having to trace backwards to document it later

This revision now requires changes to proceed.Nov 9 2023, 1:37 PM
atul requested review of this revision.Nov 9 2023, 2:18 PM

Created a Linear issue here: https://linear.app/comm/issue/ENG-5692/jest-issue-with-derived-tshape-validators

Ideally would prefer to land this as is to unblock and proceed w/ Flow and refactoring work if there's no obvious solution

ashoat requested changes to this revision.Nov 9 2023, 3:21 PM

I strongly suspect that you have a circular dependency issue and you are papering over it. I spent about 20min pulling down your changes locally and then removing this diff. I can see it failing once you introduce the hasPermission check in thread-utils.js. Then I looked through the code and I think there's an import cycle here:

thread-utils.js -> minimally-encoded-thread-permissions.js -> types/thread-types.js -> utils/entity-text.js -> thread-utils.js

Circular dependencies will show up in a Node environment (like the one keyserver and Jest run), but won't show up in "bundled" environments (like web and native). The tests failing is pointing to a larger issue, which I suspect would show up in the keyserver environment once you start importing hasPermission there.

Either way we should try to avoid them. Can you try to fix the import cycle? If pulling hasPermission outside of minimally-encoded-thread-permissions.js doesn't solve it, you could try pulling threadNoun outside of thread-utils.js.

This revision now requires changes to proceed.Nov 9 2023, 3:21 PM
atul requested review of this revision.Nov 10 2023, 1:23 PM
This revision is now accepted and ready to land.Nov 11 2023, 10:58 AM