Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(types): add type definitions #24

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// JsonML
// see: http://www.jsonml.org/syntax/
//
// `Node` type imported from lib.dom
//
/// <reference lib="dom"/>
Copy link
Author

@pgoldrbx pgoldrbx Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


export type AttributeValue = any;

export interface Attributes {
[key: string]: AttributeValue;
}
Copy link
Author

@pgoldrbx pgoldrbx Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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

  2. 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.

Copy link

@jeromecovington jeromecovington Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I frequently import a more generally named type as a "namespaced" import name if need be, I think you be alright to name simply.
  2. It might be nice to see this winnowed down to something other than any. Perhaps a primitive union named Attribute?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I've gone w/ the generic naming.
  2. 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? 🤷‍♂

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@pgoldrbx pgoldrbx Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@pgoldrbx pgoldrbx Feb 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


type TagName = string;

type TextNode = string;

export type JSONMLElement = [ TagName ]
| [ TagName, Attributes ]
| [ TagName, ...JSONMLNode[] ]
| [ TagName, Attributes, ...JSONMLNode[] ]
;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export type JSONMLNode = JSONMLElement | TextNode


// Utils
export function isFragment(jml: JSONMLElement): boolean;

export function isMarkup(value: any): boolean;

export function getTagName(jml: JSONMLElement): TagName;

export function isElement(value: any): boolean;

export function isAttributes(value: any): boolean;

export function hasAttributes(jml: JSONMLElement): boolean;

export function getAttributes(jml: JSONMLElement, addIfMissing?: boolean): Attributes;

export function addAttributes(jml: JSONMLElement, attr: Attributes): void;

export function getAttribute(jml: JSONMLElement, key: string): AttributeValue | void;

export function setAttribute(jml: JSONMLElement, key: string, value: AttributeValue): void;

export function appendChild(jml: JSONMLElement, child: JSONMLElement): boolean;

export function getChildren(jml: JSONMLElement): JSONMLNode[];


// DOM
export type JSONMLElementFilter = (jml: JSONMLElement, elem: JSONMLNode) => JSONMLElement | null;

export function fromHTML(elem: Node, filter?: JSONMLElementFilter): JSONMLElement | null;

export function fromHTMLText(html: string, filter?: JSONMLElementFilter): JSONMLElement | null;


// HTML
export class Markup {
value: string
toString(): string
}

export type HTMLElementFilter = (elem: Node) => Node

export type ErrorHandler = (err: Error, jml: JSONMLElement, filter?: HTMLElementFilter) => any

export function raw(value: string): Markup;

export function isRaw(value: any): boolean;

export let onerror: null | ErrorHandler

export function patch(elem: Node, jml: JSONMLElement, filter?: HTMLElementFilter): Node

export function toHTML(jml: string | JSONMLElement, filter?: HTMLElementFilter): Node

export function toHTMLText(jml: string | JSONMLElement, filter?: HTMLElementFilter): string
6 changes: 6 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 8 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
"version": "2.1.0",
"description": "JsonML-related tools.",
"main": "index.js",
"types": "index.d.ts",
"files": [
"*.md",
"dist",
"index.js"
"index.js",
"index.d.ts"
],
"scripts": {
"lint": "eslint ./lib/utils.js ./test",
Expand All @@ -15,7 +17,9 @@
"build": "babel lib --out-dir dist",
"prepublishOnly": "babel lib --out-dir dist",
"pretest": "npm run lint",
"test": "nyc mocha"
"test": "nyc mocha",
"test:types": "tsc --noEmit test/types.test.ts",
"posttest": "npm run test:types"
},
"keywords": [
"jsonml",
Expand Down Expand Up @@ -55,6 +59,7 @@
"eslint": "^6.0.0",
"eslint-config-egg": "^8.0.0",
"mocha": "^7.0.0",
"nyc": "^15.0.0"
"nyc": "^15.0.0",
"typescript": "^3.8.2"
}
}
51 changes: 51 additions & 0 deletions test/types.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import {
Attributes,
AttributeValue,
JSONMLNode,
} from '..';

import * as jsonml from '..';

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

const isMarkupYes: boolean = jsonml.isMarkup(jsonml.raw('hello'));
const isMarkupNo: boolean = jsonml.isMarkup(['div', 'hello']);

const paraTag: string = jsonml.getTagName(['p', 'some text']);
const fragTag: string = jsonml.getTagName(['', 'some fragment text']);

const foo = jsonml.getTagName([
'', // fragment
{ foo: 'bar' }, // attributes allowed here
['p', { foo: 'bar' }, 'some fragment text'],
['p', { foo: 'bar' }, 'some fragment text'],
['div', 'foo'],
'baz'
]);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const isElemYes: boolean = jsonml.isElement(['img', { href: 'https://example.com/foo.jpg' }]);
const isElemNo: boolean = jsonml.isElement({});

const isAttrsYes: boolean = jsonml.isAttributes({ foo: 'bar' });
const isAttrsNo: boolean = jsonml.isAttributes(['foo']);

const hasAttrsYes: boolean = jsonml.hasAttributes(['p', { foo: 'bar' }]);
const hasAttrsNo: boolean = jsonml.hasAttributes(['p']);

const gotAttrs: Attributes = jsonml.getAttributes(['p', { foo: 'bar' }]);
const gotAddedAttrs: Attributes = jsonml.getAttributes(['p'], true);
const gotEmptyAttrs: Attributes = jsonml.getAttributes(['p'], false);

const addAttributesRetval: void = jsonml.addAttributes(['p'], { foo: 'bar', a: 1 });

type MaybeAttributeValue = AttributeValue | void;
const gotAttr: MaybeAttributeValue = jsonml.getAttribute(['p', { foo: 'bar' }], 'foo');
const gotNoAttr: MaybeAttributeValue = jsonml.getAttribute(['p', { foo: 'bar' }], 'this-does-not-exist');

const setAttributeRetval: void = jsonml.setAttribute(['p'], 'foo', 'bar');

const childWasAppended: boolean = jsonml.appendChild(['p', { foo: 'bar'}], ['strong', 'Hello World', ['em', 'another'], 'bye' ]);

const gotChildren: JSONMLNode[] = jsonml.getChildren(['div', ['p', 'one'], ['p', 'two'], 'foo']);
12 changes: 12 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"compilerOptions": {
"module": "commonjs",
"noEmit": true,
"strict": true,
"target": "es6",
"lib": [
"es2015",
"dom"
]
}
}