feat(types): add type definitions #24

pgoldrbx · 2020-02-22T19:33:31Z

Description

Initial progress toward adding type definitions to the package. The utils module feels reasonably well satisfied, with a few questions. The DOM and HTML types are handled using lib.dom (is this done correctly?

Grammar used for reference: http://www.jsonml.org/syntax/

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore, documentation, cleanup

Related Issue

Motivation and Context

This library is being used in applications built with typescript. Those applications have begun defining JsonML types and interfaces locally when it would make more sense to move them here into this package.

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

My code follows the code style of this project.
I have updated the documentation (if required).
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

coveralls · 2020-02-22T19:34:47Z

Pull Request Test Coverage Report for Build 140

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 81.326%

Totals
Change from base Build 139:	0.0%
Covered Lines:	340
Relevant Lines:	393

💛 - Coveralls

pgoldrbx · 2020-02-22T19:38:33Z

index.d.ts

+  [key: string]: any // @TODO - can this be anything but a string? it technically SHOULD be,
+      // but I'm not sure if we want to be that strict. maybe it should be primitives at least?
+      // or maybe we just leave it alone?
+}


I'm unsure about naming in general - whether to be verbose like I've done here, or to be simple like Attributes and Node or Element etc

attribute values are a bit tricky. Anything parsed from an HTML will be in string format, always. Any JsonML attributes converted back into HTML or DOM are coerced to strings.

But does that mean that, in the interim, they must be strings? Also, I know there are plenty of use cases where just pragmatically the values have been modified and altered and are definitely NOT strings. I think I'm in favor of leaving this set to any rather than imposing strictness.

I frequently import a more generally named type as a "namespaced" import name if need be, I think you be alright to name simply.

It might be nice to see this winnowed down to something other than any. Perhaps a primitive union named Attribute?

I've gone w/ the generic naming.

For values, I've defined a type to centralize the issue but I'm still debating a bit. I suspect our application usage (appending things to JsonML structures) will potentially conflict with the strict value here. Pragmatically, I worry this could cause some problems for us in practice and I'm wary of implementing it. In theory, any value could be ok if it as a toString() implementation anyway. This library could export both types for consuming applications to optionally make use of? 🤷‍♂

I'm suggesting, for example:

type Attribute = string | number | <other primitive types here>;

The problem I see with using any is you open the doors to object literals and on and on, things which will not, to your point, have a toString() implementation.

question: Ah...I see you have AttributeValueStrict now. Why are you not using it?

I'm not using it because I think it will break our applications.

I'm pretty sure we are extending element attributes with objects within our applications, since we don't intend to transform the JsonML back into HTML or DOM. An example of a common use case might be an embedded content reference where later we merge in the related data structures directly with the attributes.

A likely use case, in the wild:

['inline-embed', { type: 'foo', relatedData: {/* some object */}, }]

Enforcing this type strictness is correct per the spec but might become a problem for us.

I gave this some more thought. This fork is open-source and shouldn't care about how downstream applications. The type definitions should be strict and accurate. If we need to find some workarounds later, we can figure that out. I've updated the attribute value definition accordingly.

Update: still more struggle. I'm testing this and it does indeed cause a great deal of trouble. Far more than it's worth, I'd say. Once I define strict attribute values, this is required by the Element definition and used everywhere. As a result, if an application deviates from the expected attribute value, everything fails and the application won't be able to make use of these exported definitions (they will have to be duplicated and modified for application use).

Thoughts? I feel like the most strictly correct thing to do is to keep the strict definitions and then duplicate and modify the definitions file as needed by applications. However, I worry about the trade-off in value.

In my experience, its pretty common to have strict types imported as correctly typed by a lib and then extend, union, or partial them in my code. Here's a paired down example of how this might work in this case: Playground Link.

Right, agreed. The trouble is that the AttributeValue will be used as part of other type definitions in this file. That's where it all goes sideways.

@jeromecovington @pgoldrbx yeah, I think for now we may just want to use any for pragmatic reasons since downstream consumers haven't adhered to the spec when performing manual transformations of the JsonML after the initial document conversion to JsonML from Markdown. If we're wanting to strictly adhere to the spec, we may want to do this in a two-phase approach, where the first is to publish a version using any here and then issue a breaking change that follows the spec, giving consumers a chance to adjust approach on those not-to-spec implementations

pgoldrbx · 2020-02-26T17:54:08Z

index.d.ts

+
+export interface JsonMLNode extends Array<string | JsonMLAttributes | JsonMLChildNode> {
+  0: string;
+}


I think I worked this out finally!

question: Do you actually need separate types here? Why not something like:

interface ElementMaybeWithAttributes extends Array<TagName | Attributes | Element | undefined> { 0: TagName; 1?: Attributes; }

pgoldrbx · 2020-02-22T19:44:19Z

test/types.test.ts

+
+// Utils
+const isFragYes: boolean = utils.isFragment(['', 'hi']);
+const isFragNo: boolean = utils.isFragment(['div', 'hi']);


Is there a better way to assert types?

suggestion: You might like to take a look at using assertion functions. There are various options.

suggestion: Another less obvious option would be to add a test script which exercises the types using the TypeScript compiler. Here's an example from core-logbus-sdk.

I didn't make use of assert since (currently) this file is only run through tsc and not executed at runtime. Additionally, I guess I'd have to figure out how to specify assert/node types in the config?

I did look at other repo examples, and the logbus is another good reference
https://github.com/CondeNast-Copilot/core-logbus-sdk/blob/master/test/assert-types.ts

It feels somewhat similar to what I'm doing - which is essentially just trying to setup all the calls and objects. I went the extra difference of assigning the return value so the type checker would catch this also.

jeromecovington

Few initial thoughts and comments, hopefully they are useful!

jeromecovington · 2020-02-24T16:50:38Z

index.d.ts

+  [key: string]: any // @TODO - can this be anything but a string? it technically SHOULD be,
+      // but I'm not sure if we want to be that strict. maybe it should be primitives at least?
+      // or maybe we just leave it alone?
+}


I frequently import a more generally named type as a "namespaced" import name if need be, I think you be alright to name simply.

It might be nice to see this winnowed down to something other than any. Perhaps a primitive union named Attribute?

index.d.ts

+
+export interface JsonMLNode extends Array<string | JsonMLAttributes | JsonMLChildNode> {
+  0: string;
+}


jeromecovington · 2020-02-24T17:05:09Z

index.d.ts

+
+  export function addAttributes(jml: JsonMLNode, attr: JsonMLAttributes): undefined;
+
+  export function getAttribute(jml: JsonMLNode, key: string): any;


suggestion: Here, you could use the suggested Attribute as return type.

jeromecovington · 2020-02-24T17:06:01Z

index.d.ts

+
+  export function getAttributes(jml: JsonMLNode, addIfMissing?: boolean): JsonMLAttributes;
+
+  export function addAttributes(jml: JsonMLNode, attr: JsonMLAttributes): undefined;


suggestion: I may be mistaken but I think the return value here would more properly be void. I see this elsewhere in the utils namespace as well.

oh yeah! thanks

jeromecovington · 2020-02-24T17:10:34Z

test/types.test.ts

+
+// Utils
+const isFragYes: boolean = utils.isFragment(['', 'hi']);
+const isFragNo: boolean = utils.isFragment(['div', 'hi']);


suggestion: You might like to take a look at using assertion functions. There are various options.

jeromecovington · 2020-02-24T17:13:15Z

test/types.test.ts

+
+// Utils
+const isFragYes: boolean = utils.isFragment(['', 'hi']);
+const isFragNo: boolean = utils.isFragment(['div', 'hi']);


suggestion: Another less obvious option would be to add a test script which exercises the types using the TypeScript compiler. Here's an example from core-logbus-sdk.

pgoldrbx · 2020-03-09T16:37:10Z

index.d.ts

+//
+// `Node` type imported from lib.dom
+//
+/// <reference lib="dom"/>


Attempting to mark that lib.dom is required. The Node type is available as a global after adding 'dom' to tsconfig. I wish I could reference it more explicitly.

Overall, I'm not sure if I'm handling namespacing/scope properly.

I've also renamed types to avoid shadowing the DOM globals.

Element -> JSONMLElement

Node -> JSONMLNode

(based on similar from HTMLElement)

pgoldrbx · 2020-03-09T16:37:57Z

index.d.ts

+  | [ TagName, Attributes ]
+  | [ TagName, ...JSONMLNode[] ]
+  | [ TagName, Attributes, ...JSONMLNode[] ]
+  ;


@jeromecovington I've tried another approach! I think I have variable-length tuples working now by using spread. 🤷‍♂

pgoldrbx · 2020-03-09T16:40:29Z

test/types.test.ts

+  ['p', { foo: 'bar' }, 'some fragment text'],
+  ['div', 'foo'],
+  'baz'
+]);


Tested and can verify that I get issues if the attributes are moved to the wrong position or if an undefined or otherwise invalid element is added somewhere.

pgoldrbx

@neilius @jeromecovington could I get another review on this?

pgoldrbx · 2024-02-07T16:33:18Z

closing as abandoned

pgoldrbx added the help wanted Extra attention is needed label Feb 22, 2020

pgoldrbx requested a review from neilius February 22, 2020 19:33

pgoldrbx commented Feb 22, 2020

View reviewed changes

pgoldrbx force-pushed the feat/types branch from 094cfda to 15cbc7e Compare February 22, 2020 19:43

pgoldrbx commented Feb 22, 2020

View reviewed changes

pgoldrbx requested review from jeromecovington and natemcdowel February 22, 2020 19:45

jeromecovington reviewed Feb 24, 2020

View reviewed changes

pgoldrbx force-pushed the feat/types branch 3 times, most recently from 140d314 to 5a51905 Compare February 29, 2020 05:22

pgoldrbx force-pushed the feat/types branch from 5a51905 to 425e4a3 Compare March 9, 2020 16:34

pgoldrbx commented Mar 9, 2020

View reviewed changes

pgoldrbx changed the title ~~feat(types): add type definitions (in progress)~~ feat(types): add type definitions Mar 16, 2020

pgoldrbx marked this pull request as ready for review March 16, 2020 23:26

pgoldrbx added enhancement New feature or request and removed help wanted Extra attention is needed labels Mar 16, 2020

pgoldrbx added 9 commits March 25, 2020 18:35

feat(types): add type definitions

5a75bc2

refactor: modules are not exported as namespaced, improve type defs

184d4a2

use strict types for attribute values

2ebc320

additional cleanup

f9f9cd0

more experimentation

b7403ec

better namespacing and use lib.dom for Node type

6e5ae64

better JSONMLElement union with spread

9b31b18

feat(types): add isMarkup

6b4ed7c

fix(types): remove strict types for AttributeValue

acfdaf9

pgoldrbx force-pushed the feat/types branch from 425e4a3 to acfdaf9 Compare March 25, 2020 22:38

pgoldrbx commented Mar 25, 2020

View reviewed changes

pgoldrbx closed this Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(types): add type definitions #24

feat(types): add type definitions #24

pgoldrbx commented Feb 22, 2020 •

edited

Loading

coveralls commented Feb 22, 2020 •

edited

Loading

pgoldrbx Feb 22, 2020 •

edited

Loading

jeromecovington Feb 24, 2020 •

edited

Loading

pgoldrbx Feb 26, 2020

jeromecovington Feb 26, 2020

jeromecovington Feb 26, 2020

pgoldrbx Feb 26, 2020 •

edited

Loading

pgoldrbx Feb 29, 2020 •

edited

Loading

jeromecovington Mar 2, 2020

pgoldrbx Mar 3, 2020

neilius Mar 16, 2020

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

pgoldrbx Feb 26, 2020

jeromecovington Mar 2, 2020

pgoldrbx Feb 22, 2020

jeromecovington Feb 24, 2020

jeromecovington Feb 24, 2020

pgoldrbx Feb 24, 2020

jeromecovington left a comment

jeromecovington Feb 24, 2020 •

edited

Loading

This comment was marked as resolved.

jeromecovington Feb 24, 2020

jeromecovington Feb 24, 2020

pgoldrbx Feb 24, 2020

jeromecovington Feb 24, 2020

jeromecovington Feb 24, 2020

pgoldrbx Mar 9, 2020 •

edited

Loading

pgoldrbx Mar 9, 2020

pgoldrbx Mar 9, 2020

pgoldrbx left a comment

pgoldrbx commented Feb 7, 2024


		export function addAttributes(jml: JsonMLNode, attr: JsonMLAttributes): undefined;

		export function getAttribute(jml: JsonMLNode, key: string): any;


		export function getAttributes(jml: JsonMLNode, addIfMissing?: boolean): JsonMLAttributes;

		export function addAttributes(jml: JsonMLNode, attr: JsonMLAttributes): undefined;

feat(types): add type definitions #24

feat(types): add type definitions #24

Conversation

pgoldrbx commented Feb 22, 2020 • edited Loading

Description

Types of changes

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

coveralls commented Feb 22, 2020 • edited Loading

Pull Request Test Coverage Report for Build 140

💛 - Coveralls

pgoldrbx Feb 22, 2020 • edited Loading

Choose a reason for hiding this comment

jeromecovington Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgoldrbx Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

pgoldrbx Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromecovington left a comment

Choose a reason for hiding this comment

jeromecovington Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgoldrbx Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgoldrbx left a comment

Choose a reason for hiding this comment

pgoldrbx commented Feb 7, 2024

pgoldrbx commented Feb 22, 2020 •

edited

Loading

coveralls commented Feb 22, 2020 •

edited

Loading

pgoldrbx Feb 22, 2020 •

edited

Loading

jeromecovington Feb 24, 2020 •

edited

Loading

pgoldrbx Feb 26, 2020 •

edited

Loading

pgoldrbx Feb 29, 2020 •

edited

Loading

jeromecovington Feb 24, 2020 •

edited

Loading

pgoldrbx Mar 9, 2020 •

edited

Loading