Page MenuHomePhabricator

Refactor lib/facts files from JSON to JS
ClosedPublic

Authored by rohan on Sep 9 2022, 9:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 3:02 AM
Unknown Object (File)
Thu, Dec 19, 1:17 AM
Unknown Object (File)
Thu, Dec 19, 1:15 AM
Unknown Object (File)
Thu, Dec 19, 1:12 AM
Unknown Object (File)
Thu, Dec 19, 1:04 AM
Unknown Object (File)
Thu, Dec 19, 1:02 AM
Unknown Object (File)
Thu, Dec 19, 12:51 AM
Unknown Object (File)
Thu, Dec 19, 12:51 AM

Details

Summary

This commit deals with ENG-1181, part of the parent issue ENG-929. ENG-1181 handles refactoring the lib/facts files from JSON into JS, and fixing some import statements.

Commandeering this revision from some previous work.

Test Plan

Both Web and Native seem functional to me. Messages still load as expected, and created several new accounts to test the Genesis bot (results are identical with these changes and on the current master branch).

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D5107
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 9 2022, 9:51 PM
Harbormaster failed remote builds in B12068: Diff 16568!

Initial Commit that deals with Eng 1181, upgrading Node version and removing json imports

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 15 2022, 5:06 PM
Harbormaster failed remote builds in B12190: Diff 16721!
Harbormaster returned this revision to the author for changes because remote builds failed.Sep 22 2022, 1:24 PM
Harbormaster failed remote builds in B12395: Diff 17012!

Changed import json to use fs instead of import (ENG-929)

In order to upgrade node version, we must stray away from experimental features like import for json.

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 22 2022, 3:55 PM
Harbormaster failed remote builds in B12397: Diff 17015!

looks like I was added because the initial diff made some changes to native/cpp, going to resign, as I'm not qualified to review this

ashoat added a reviewer: dev.
ashoat foisted this revision upon atul.
ashoat edited reviewers, added: ashoat; removed: atul.
ashoat removed reviewers: jon, dev.
ashoat requested changes to this revision.Sep 23 2022, 1:31 PM
ashoat added inline comments.
keyserver/src/scripts/add-edit-thread-detailed-permissions.js
1 ↗(On Diff #17015)

Add newline after // @flow

lib/facts/ashoat.js
1 ↗(On Diff #17015)

Add newline after // @flow

3–5 ↗(On Diff #17015)

$ReadOnly

lib/facts/bots.js
1 ↗(On Diff #17015)

Add newline after // @flow

2–4 ↗(On Diff #17015)

$ReadOnly

3 ↗(On Diff #17015)

Fix any type

lib/facts/genesis.js
1 ↗(On Diff #17015)

Add newline after // @flow

2–7 ↗(On Diff #17015)

$ReadOnly

6 ↗(On Diff #17015)

$ReadOnlyArray

lib/facts/staff.js
1 ↗(On Diff #17015)

Add newline after // @flow

lib/facts/testers.js
1 ↗(On Diff #17015)

Add newline after // @flow

2–6 ↗(On Diff #17015)

$ReadOnly

5 ↗(On Diff #17015)

$ReadOnlyArray

This revision now requires changes to proceed.Sep 23 2022, 1:31 PM
atul added a reviewer: atul.

Took over this revision, addressed comments on previous revision.

I think you might have created a new diff rather than amending the existing diff

Can you squash the two together and then update this diff?

lib/facts/bots.js
4 ↗(On Diff #17114)

Just wanted to check something here: my understanding is that I can't have a ReadOnly property apply to all nested objects, is this correct? I saw something like this, but it seems to just be a feature request

In D5107#154273, @atul wrote:

Can you squash the two together and then update this diff?

Ah, let me give that a go

Refactor lib/facts files from JSON to JS

rohan retitled this revision from Changed json fact files to js to Refactor lib/facts files from JSON to JS.Sep 27 2022, 10:32 AM
rohan edited the summary of this revision. (Show Details)
rohan edited the test plan for this revision. (Show Details)
keyserver/src/utils/import-json.js
48

This seems to address ENG-1182 unless I'm mistaken. If it does, should we leave this as is or should I make revert this change and put up a separate diff for it?

Patched in the changes, and everything seems to work as expected, good work

Refactor $ReadOnly< ... > syntax to be more consistent with the codebase and separate ENG-1182 changes out of this diff (following sync with Atul)

Great work!! This diff looks clean, and I'm glad you separate out the changes to import-json.js to a different diff.

One thing that is new from when we created the Linear issue: we now have lib/shared/comm-icon-config.json and lib/shared/swmansion-icon-config.json, which are both being accessed from both web and native. I think things should be fine as long as keyserver doesn't import the web file directly... and those will be annoying to migrate since they're auto-generated. But figured I'd note it here. cc @atul who I think initially introduced both

This revision is now accepted and ready to land.Sep 29 2022, 5:35 AM

Great work!! This diff looks clean, and I'm glad you separate out the changes to import-json.js to a different diff.

One thing that is new from when we created the Linear issue: we now have lib/shared/comm-icon-config.json and lib/shared/swmansion-icon-config.json, which are both being accessed from both web and native. I think things should be fine as long as keyserver doesn't import the web file directly... and those will be annoying to migrate since they're auto-generated. But figured I'd note it here. cc @atul who I think initially introduced both

Thanks for the heads up! I'll land this to not block it, but if those two JSON files do become an issue, I'm happy to try to deal with it in a separate diff

Rebase and fix merge conflicts

This revision was automatically updated to reflect the committed changes.