Port config forms JS codebase to TypeScript/spina.
Review Request #13140 — Created July 13, 2023 and submitted
Information | |
---|---|
david | |
Djblets | |
release-4.x | |
Reviewers | |
djblets | |
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 theListItem
implementation allowed
passing in anactions
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 usesetActions
), but I've
added some compatibility code to emit a deprecation warning just in the
very unlikely case that someone was using this.
Ran js-tests.
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
I think this should say "This operates just like a..."? |
![]() |
|
Should say "stores" instead of "stored". |
![]() |
|
4.0? Same below. |
|
|
suite actually comes from us. |
|
|
Should use double ticks around "template" |
![]() |
|
I think we need to map this to an explicit type mapping strings -> strings for subclasses to extend, otherwise … |
|
|
Should document this and any other instance variables. |
![]() |
|
Can we add docs to these? The public ones at least. |
|
|
It'd be nice to have a type for this that can be imported. |
|
|
Since this is for returning any arbitrary mapping of data for use during render, I don't think we want unknown. … |
|
|
This should end with ,, and ) { on the next line. |
|
|
Missing docs. |
|
|
Can we add docs for these? |
|
|
Can we add docs for at least public variables? |
|
|
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. … |
|
|
Can you decorate Backbone.Collection with :js:class? |
|
|
I think these have to use 1. and 2. to format correctly in ReStructuredText. |
|
|
Can you decorate fetch with :js:meth? |
|

-
-
djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 1) I think this should say "This operates just like a..."?
-
djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 1) Should say "stores" instead of "stored".
-
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1) Should use double ticks around "template"
-
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1) Should document this and any other instance variables.
-
-
-
djblets/static/djblets/js/configForms/models/tests/listItemModelTests.ts (Diff revision 1) suite
actually comes from us. -
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1) 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) Can we add docs to these? The public ones at least.
-
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revision 1) 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) This should end with
,
, and) {
on the next line. -
-
djblets/static/djblets/js/configForms/views/listView.ts (Diff revision 1) Can we add docs for these?
-
djblets/static/djblets/js/configForms/views/pagesView.ts (Diff revision 1) Can we add docs for at least public variables?
-
djblets/static/djblets/js/tests/index.ts (Diff revision 1) 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: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+1806 -1138) |
Checks run (2 succeeded)
-
-
djblets/static/djblets/js/configForms/views/listItemView.ts (Diff revisions 1 - 2) 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) Can you decorate
Backbone.Collection
with:js:class
? -
djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 2) I think these have to use
1.
and2.
to format correctly in ReStructuredText. -
djblets/static/djblets/js/configForms/collections/listItemsCollection.ts (Diff revision 2) Can you decorate
fetch
with:js:meth
?
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+1814 -1140) |