Details

Reviewers

ashoat
atul
ginsu

Commits

rCOMMbccb9fff1e58: Refactor lib/facts files from JSON to JS

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_1

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• dev created this revision.Sep 9 2022, 9:47 PM

Herald added a reviewer: • jon. · View Herald TranscriptSep 9 2022, 9:47 PM

Herald added subscribers: • abosh, tomek, ashoat. · View Herald Transcript

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!

Working flow build

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!

Harbormaster completed remote builds in B12397: Diff 17015.Sep 23 2022, 7:35 AM

• dev requested review of this revision.Sep 23 2022, 7:35 AM

Harbormaster completed remote builds in B12395: Diff 17012.Sep 23 2022, 7:38 AM

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 commandeered this revision.Sep 23 2022, 1:29 PM

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	Add newline after `// @flow`
lib/facts/ashoat.js
1	Add newline after `// @flow`
3–5	`$ReadOnly`
lib/facts/bots.js
1	Add newline after `// @flow`
2–4	`$ReadOnly`
3	Fix `any` type
lib/facts/genesis.js
1	Add newline after `// @flow`
2–7	`$ReadOnly`
6	`$ReadOnlyArray`
lib/facts/staff.js
1	Add newline after `// @flow`
lib/facts/testers.js
1	Add newline after `// @flow`
2–6	`$ReadOnly`
5	`$ReadOnlyArray`

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

atul foisted this revision upon rohan.Sep 26 2022, 9:58 AM

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?

rohan added inline comments.Sep 27 2022, 10:08 AM

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

Harbormaster failed remote builds in B12467: Diff 17114!Sep 27 2022, 10:13 AM

Refactor lib/facts files from JSON to JS

Harbormaster failed remote builds in B12468: Diff 17115!Sep 27 2022, 10:24 AM

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)

rohan added inline comments.Sep 27 2022, 10:42 AM

keyserver/src/utils/import-json.js
48 ↗	(On Diff #17115)	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?

Harbormaster completed remote builds in B12467: Diff 17114.Sep 27 2022, 12:55 PM

Harbormaster completed remote builds in B12468: Diff 17115.

atul added a reviewer: ginsu.Sep 27 2022, 1:35 PM

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)

Harbormaster completed remote builds in B12494: Diff 17149.Sep 28 2022, 9:51 AM

Looks good!

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

In D5107#154944, @ashoat wrote:

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

Harbormaster completed remote builds in B12539: Diff 17204.Sep 29 2022, 11:51 AM

Closed by commit rCOMMbccb9fff1e58: Refactor lib/facts files from JSON to JS (authored by rohan). · Explain WhySep 29 2022, 11:53 AM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMMbccb9fff1e58: Refactor lib/facts files from JSON to JS.

Refactor lib/facts files from JSON to JS
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 17015

docs/dev_environment.md

keyserver/src/scripts/add-edit-thread-detailed-permissions.js

keyserver/src/scripts/fix-new-thread-types.js

keyserver/src/scripts/sidebar-know-of-migration.js

keyserver/src/utils/import-json.js

lib/facts/ashoat.js

lib/facts/ashoat.json

lib/facts/bots.js

lib/facts/bots.json

lib/facts/genesis.js

lib/facts/genesis.json

lib/facts/staff.js

lib/facts/staff.json

lib/facts/testers.js

lib/facts/testers.json

Refactor lib/facts files from JSON to JSClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 17015

docs/dev_environment.md

keyserver/src/scripts/add-edit-thread-detailed-permissions.js

keyserver/src/scripts/fix-new-thread-types.js

keyserver/src/scripts/sidebar-know-of-migration.js

keyserver/src/utils/import-json.js

lib/facts/ashoat.js

lib/facts/ashoat.json

lib/facts/bots.js

lib/facts/bots.json

lib/facts/genesis.js

lib/facts/genesis.json

lib/facts/staff.js

lib/facts/staff.json

lib/facts/testers.js

lib/facts/testers.json

Refactor lib/facts files from JSON to JS
ClosedPublic
Actions

Revision Contents
Changeset List