Summary

Port config forms JS codebase to TypeScript/spina.

Review Request #13140 — Created July 13, 2023 and submitted Aug. 3, 2023, 11:56 a.m.

Information

Owner

david

Repository

Djblets

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

This change ports the JavaScript side of the config forms app to use
TypeScript and spina.

For the most part this is a very straightforward port. The only annoying
thing was I discovered that the ListItem implementation allowed
passing in an actions parameter which was treated as an option arg,
but was actually being passed in as part of the attributes (despite not
being an attribute on the model). This was only used inside of our unit
tests (which have themselves been updated to use setActions), but I've
added some compatibility code to emit a deprecation warning just in the
very unlikely case that someone was using this.

Testing Done

Ran js-tests.

Commits

Summary	ID
Port config forms JS codebase to TypeScript/spina. This change ports the JavaScript side of the config forms app to use TypeScript and spina. For the most part this is a very straightforward port. The only annoying thing was I discovered that the `ListItem` implementation allowed passing in an `actions` parameter which was treated as an option arg, but was actually being passed in as part of the attributes (despite not being an attribute on the model). This was only used inside of our unit tests (which have themselves been updated to use `setActions`), but I've added some compatibility code to emit a deprecation warning just in the very unlikely case that someone was using this. Testing Done: Ran js-tests.	67d63b3d82d46d0d02a1a17da7d20ed28ba7d5e1

Summary

Port config forms JS codebase to TypeScript/spina.

This change ports the JavaScript side of the config forms app to use TypeScript and spina. For the most part this is a very straightforward port. The only annoying thing was I discovered that the `ListItem` implementation allowed passing in an `actions` parameter which was treated as an option arg, but was actually being passed in as part of the attributes (despite not being an attribute on the model). This was only used inside of our unit tests (which have themselves been updated to use `setActions`), but I've added some compatibility code to emit a deprecation warning just in the very unlikely case that someone was using this. Testing Done: Ran js-tests.

67d63b3d82d46d0d02a1a17da7d20ed28ba7d5e1

Issues

Description	From	Last Updated
I think this should say "This operates just like a..."?	maubin	July 28, 2023, 1:36 p.m.
Should say "stores" instead of "stored".	maubin	July 28, 2023, 1:36 p.m.
4.0? Same below.	chipx86	July 28, 2023, 1:37 p.m.
suite actually comes from us.	chipx86	July 31, 2023, 9:32 a.m.
Should use double ticks around "template"	maubin	July 28, 2023, 1:36 p.m.
I think we need to map this to an explicit type mapping strings -> strings for subclasses to extend, otherwise …	chipx86	July 28, 2023, 1:37 p.m.
Should document this and any other instance variables.	maubin	July 28, 2023, 1:37 p.m.
Can we add docs to these? The public ones at least.	chipx86	July 28, 2023, 1:37 p.m.
It'd be nice to have a type for this that can be imported.	chipx86	Aug. 2, 2023, 9:43 a.m.
Since this is for returning any arbitrary mapping of data for use during render, I don't think we want unknown. …	chipx86	July 28, 2023, 1:38 p.m.
This should end with ,, and ) { on the next line.	chipx86	July 28, 2023, 1:38 p.m.
Missing docs.	chipx86	July 28, 2023, 1:38 p.m.
Can we add docs for these?	chipx86	July 28, 2023, 1:40 p.m.
Can we add docs for at least public variables?	chipx86	July 28, 2023, 1:40 p.m.
Let's follow the pattern in Review Board and have a tests/index.ts in each {models,views}/tests/ directory, and just import that directory. …	chipx86	July 28, 2023, 1:41 p.m.
Can you decorate Backbone.Collection with :js:class?	chipx86	Aug. 2, 2023, 9:43 a.m.
I think these have to use 1. and 2. to format correctly in ReStructuredText.	chipx86	Aug. 2, 2023, 9:43 a.m.
Can you decorate fetch with :js:meth?	chipx86	Aug. 2, 2023, 9:44 a.m.

flake8 passed.

JSHint passed.

djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
I think this should say "This operates just like a..."?
```
djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should say "stores" instead of "stored".
```
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should use double ticks around "template"
```
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Should document this and any other instance variables.
```

djblets/static/djblets/js/configForms/models/listItemModel.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
4.0?
Same below.
```
djblets/static/djblets/js/configForms/models/tests/listItemModelTests.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
suite actually comes from us.
```
1. david July 31, 2023, 9:37 a.m.
  Just going to remove this for now until I come up with a solution.

djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1)

The issue has been resolved. Show all issues

I think we need to map this to an explicit type mapping strings -> strings for subclasses to extend, otherwise it'll be limited to these values.

djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we add docs to these? The public ones at least.
```

djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1)

The issue has been resolved. Show all issues

Since this is for returning any arbitrary mapping of data for use during render, I don't think we want unknown. I think we want an interface that maps any string key to any value.

djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
This should end with ,, and ) { on the next line.
```
djblets/static/djblets/js/configForms/views/listView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing docs.
```
djblets/static/djblets/js/configForms/views/listView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we add docs for these?
```
djblets/static/djblets/js/configForms/views/pagesView.ts (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we add docs for at least public variables?
```

djblets/static/djblets/js/tests/index.ts (Diff revision 1)

The issue has been resolved. Show all issues

Let's follow the pattern in Review Board and have a tests/index.ts in each {models,views}/tests/ directory, and just import that directory.
For example, Review Board's js/tests/index.ts has:

import '../common/models/tests';
import '../common/resources/collections/tests';
import '../common/resources/models/tests';
import '../common/views/tests';
import '../reviews/models/tests';
import '../reviews/views/tests';
import '../ui/views/tests';

Commits:

	Summary	ID
	Port config forms JS codebase to TypeScript/spina. This change ports the JavaScript side of the config forms app to use TypeScript and spina. For the most part this is a very straightforward port. The only annoying thing was I discovered that the `ListItem` implementation allowed passing in an `actions` parameter which was treated as an option arg, but was actually being passed in as part of the attributes (despite not being an attribute on the model). This was only used inside of our unit tests (which have themselves been updated to use `setActions`), but I've added some compatibility code to emit a deprecation warning just in the very unlikely case that someone was using this. Testing Done: Ran js-tests.	6ff9a26b8e11d5b691c9c89f78dfd1eb7c628b31
	Port config forms JS codebase to TypeScript/spina. This change ports the JavaScript side of the config forms app to use TypeScript and spina. For the most part this is a very straightforward port. The only annoying thing was I discovered that the `ListItem` implementation allowed passing in an `actions` parameter which was treated as an option arg, but was actually being passed in as part of the attributes (despite not being an attribute on the model). This was only used inside of our unit tests (which have themselves been updated to use `setActions`), but I've added some compatibility code to emit a deprecation warning just in the very unlikely case that someone was using this. Testing Done: Ran js-tests.	32853981b0755bb4adfc93b1510b40c92735de31

Diff:

Revision 2 (+1806 -1138)

Show changes

	djblets/staticbundles.py
	djblets/static/djblets/js/configForms/index.ts
	djblets/static/djblets/js/configForms/collections/listItemsCollection.ts
	djblets/static/djblets/js/configForms/collections/listItemsCollection.es6.js
	djblets/static/djblets/js/configForms/models/listItemModel.ts
	djblets/static/djblets/js/configForms/models/listItemModel.es6.js
	djblets/static/djblets/js/configForms/models/listModel.ts
	djblets/static/djblets/js/configForms/models/listModel.js
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
It'd be nice to have a type for this that can be imported.
```
djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you decorate Backbone.Collection with :js:class?
```

djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 2)

The issue has been resolved. Show all issues

I think these have to use 1. and 2. to format correctly in ReStructuredText.

djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you decorate fetch with :js:meth?
```

Commits:

	Summary	ID
	Port config forms JS codebase to TypeScript/spina. This change ports the JavaScript side of the config forms app to use TypeScript and spina. For the most part this is a very straightforward port. The only annoying thing was I discovered that the `ListItem` implementation allowed passing in an `actions` parameter which was treated as an option arg, but was actually being passed in as part of the attributes (despite not being an attribute on the model). This was only used inside of our unit tests (which have themselves been updated to use `setActions`), but I've added some compatibility code to emit a deprecation warning just in the very unlikely case that someone was using this. Testing Done: Ran js-tests.	32853981b0755bb4adfc93b1510b40c92735de31
	Port config forms JS codebase to TypeScript/spina. This change ports the JavaScript side of the config forms app to use TypeScript and spina. For the most part this is a very straightforward port. The only annoying thing was I discovered that the `ListItem` implementation allowed passing in an `actions` parameter which was treated as an option arg, but was actually being passed in as part of the attributes (despite not being an attribute on the model). This was only used inside of our unit tests (which have themselves been updated to use `setActions`), but I've added some compatibility code to emit a deprecation warning just in the very unlikely case that someone was using this. Testing Done: Ran js-tests.	67d63b3d82d46d0d02a1a17da7d20ed28ba7d5e1

Diff:

Revision 3 (+1814 -1140)

Show changes

	djblets/staticbundles.py
	djblets/static/djblets/js/configForms/index.ts
	djblets/static/djblets/js/configForms/collections/listItemsCollection.ts
	djblets/static/djblets/js/configForms/collections/listItemsCollection.es6.js
	djblets/static/djblets/js/configForms/models/listItemModel.ts
	djblets/static/djblets/js/configForms/models/listItemModel.es6.js
	djblets/static/djblets/js/configForms/models/listModel.ts
	djblets/static/djblets/js/configForms/models/listModel.js
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (938d76c)