Summary

Fix up import paths and groups in new TypeScript code.

Review Request #13171 — Created Aug. 1, 2023 and submitted Aug. 7, 2023, 9:19 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-6.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

As we've defined how our JS codebase is going to work going forward,

we've not been terribly disciplined about how we do imports. This change

makes everything consistent with three import groups (3rd party, other

modules, and the current module) that kind of matches the way that our

Python code works. Where we can, this has been simplified to import

everything from the top-level indexes, which makes sorting things

alphabetically by module name much easier.

Testing Done

Ran js-tests.

Commits

Summary	ID
Fix up import paths and groups in new TypeScript code. As we've defined how our JS codebase is going to work going forward, we've not been terribly disciplined about how we do imports. This change makes everything consistent with three import groups (3rd party, other modules, and the current module) that kind of matches the way that our Python code works. Where we can, this has been simplified to import everything from the top-level indexes, which makes sorting things alphabetically by module name much easier. Testing Done: Ran js-tests.	9310e38b67583e72be549e97daa9d580af4d5cc7

Summary

Fix up import paths and groups in new TypeScript code.

As we've defined how our JS codebase is going to work going forward, we've not been terribly disciplined about how we do imports. This change makes everything consistent with three import groups (3rd party, other modules, and the current module) that kind of matches the way that our Python code works. Where we can, this has been simplified to import everything from the top-level indexes, which makes sorting things alphabetically by module name much easier. Testing Done: Ran js-tests.

9310e38b67583e72be549e97daa9d580af4d5cc7

Issues

Description	From	Last Updated
This module is within this bundle, so it should use a relative import in a separate import group.	chipx86	Aug. 2, 2023, 9:22 a.m.
Should reviewboard/common be before reviewboard/common/...?	chipx86	Aug. 2, 2023, 9:22 a.m.
common should be before ui.	chipx86	Aug. 2, 2023, 9:22 a.m.
reviewRequestEditorModel sorts before reviewablePageModel (R < a).	chipx86	Aug. 2, 2023, 9:22 a.m.
This is in the same bundle, so it should use relative imports.	chipx86	Aug. 2, 2023, 9:23 a.m.
common before ui.	chipx86	Aug. 2, 2023, 9:27 a.m.
Isn't this one supposed to be a relative import since its in the same bundle?	maubin	Aug. 2, 2023, 1:50 p.m.
Should we import this one from reviewboard/common like we do with ReviewReply?	maubin	Aug. 2, 2023, 1:50 p.m.

flake8 passed.

JSHint passed.

We should document the import rules in Notion.
To make sure I have this right (maybe this would be good for the description), do higher-level paths sort before deeper paths? E.g. ../z sorts before ./a?

reviewboard/static/rb/js/reviews/models/tests/reviewablePageModelTests.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
This module is within this bundle, so it should use a relative import in a separate import group.
```
reviewboard/static/rb/js/reviews/models/unifiedBannerModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should reviewboard/common be before reviewboard/common/...?
```

reviewboard/static/rb/js/reviews/views/reviewablePageView.ts (Diff revision 1)

The issue has been resolved. Show all issues

common should be before ui.

reviewboard/static/rb/js/reviews/views/reviewablePageView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
reviewRequestEditorModel sorts before reviewablePageModel (R < a).
```

reviewboard/static/rb/js/reviews/views/tests/diffViewerPageViewTests.ts (Diff revision 1)

The issue has been resolved. Show all issues

This is in the same bundle, so it should use relative imports.

reviewboard/static/rb/js/reviews/views/tests/reviewRequestEditorViewTests.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
common before ui.
```

Commits:

	Summary	ID
	Fix up import paths and groups in new TypeScript code. As we've defined how our JS codebase is going to work going forward, we've not been terribly disciplined about how we do imports. This change makes everything consistent with three import groups (3rd party, other modules, and the current module) that kind of matches the way that our Python code works. Where we can, this has been simplified to import everything from the top-level indexes, which makes sorting things alphabetically by module name much easier. Testing Done: Ran js-tests.	2eb73decd1bcf836b588ea51eefdd75efeee1308
	Fix up import paths and groups in new TypeScript code. As we've defined how our JS codebase is going to work going forward, we've not been terribly disciplined about how we do imports. This change makes everything consistent with three import groups (3rd party, other modules, and the current module) that kind of matches the way that our Python code works. Where we can, this has been simplified to import everything from the top-level indexes, which makes sorting things alphabetically by module name much easier. Testing Done: Ran js-tests.	bd39ea0be1d19e7d7acae0c4a9ca0f83e3893df6

Diff:

Revision 2 (+196 -190)

Show changes

	reviewboard/static/rb/js/common/actions/views/menuActionView.ts
	reviewboard/static/rb/js/common/models/tests/commChannelModelTests.ts
	reviewboard/static/rb/js/common/models/tests/userSessionModelTests.ts
	reviewboard/static/rb/js/common/resources/collections/tests/resourceCollectionTests.ts
	reviewboard/static/rb/js/common/resources/models/draftResourceModelMixin.ts
	reviewboard/static/rb/js/common/resources/models/reviewModel.ts
	reviewboard/static/rb/js/common/resources/models/reviewReplyModel.ts
	reviewboard/static/rb/js/reviewRequestPage/views/reviewReplyDraftBannerView.ts
	20 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Thanks for standardizing these! Looking forward to having a document in Notion for this too.

reviewboard/static/rb/js/common/models/tests/userSessionModelTests.ts (Diff revision 2)

The issue has been dropped. Show all issues

Isn't this one supposed to be a relative import since its in the same bundle?

david Aug. 2, 2023, 1:50 p.m.

UserSession is kind of a special case, when combined with test files. The tests are their own bundle, even though we import related things relatively (though I'm wondering if perhaps we should change that). Doing a cross-bundle import into the test file and then having other things get it from the common bundle meant that we actually had two singleton instances.

reviewboard/static/rb/js/reviews/models/unifiedBannerModel.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Should we import this one from reviewboard/common like we do with ReviewReply?
```

Commits:

	Summary	ID
	Fix up import paths and groups in new TypeScript code. As we've defined how our JS codebase is going to work going forward, we've not been terribly disciplined about how we do imports. This change makes everything consistent with three import groups (3rd party, other modules, and the current module) that kind of matches the way that our Python code works. Where we can, this has been simplified to import everything from the top-level indexes, which makes sorting things alphabetically by module name much easier. Testing Done: Ran js-tests.	bd39ea0be1d19e7d7acae0c4a9ca0f83e3893df6
	Fix up import paths and groups in new TypeScript code. As we've defined how our JS codebase is going to work going forward, we've not been terribly disciplined about how we do imports. This change makes everything consistent with three import groups (3rd party, other modules, and the current module) that kind of matches the way that our Python code works. Where we can, this has been simplified to import everything from the top-level indexes, which makes sorting things alphabetically by module name much easier. Testing Done: Ran js-tests.	9310e38b67583e72be549e97daa9d580af4d5cc7

Diff:

Revision 3 (+198 -188)

Show changes

	reviewboard/static/rb/js/common/actions/views/menuActionView.ts
	reviewboard/static/rb/js/common/models/tests/commChannelModelTests.ts
	reviewboard/static/rb/js/common/models/tests/userSessionModelTests.ts
	reviewboard/static/rb/js/common/resources/collections/tests/resourceCollectionTests.ts
	reviewboard/static/rb/js/common/resources/models/draftResourceModelMixin.ts
	reviewboard/static/rb/js/common/resources/models/reviewModel.ts
	reviewboard/static/rb/js/common/resources/models/reviewReplyModel.ts
	reviewboard/static/rb/js/reviewRequestPage/views/reviewReplyDraftBannerView.ts
	20 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (1119cbf)