-
Notifications
You must be signed in to change notification settings - Fork 8
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
slimdom 2.0 #28
slimdom 2.0 #28
Conversation
Note: although the DOM standard links to the fifth edition of the XML spec, both browsers and the web platform seem to use the fourth edition rules.
This also serves to inject all constructors where possible, avoiding cyclic dependencies.
src/util/errorHelpers.ts
Outdated
|
||
export function expectObject<T>(value: T, Constructor: any): void { | ||
if (!(value instanceof Constructor)) { | ||
throw new TypeError(`Value should be an instance of ${Constructor.name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't ask how I know this, but the typescript compiler mangles these properties:
element.appendChild('PRRRRT');
=> "Value should be an instance of e"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a couple of tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one at https://github.com/bwrrp/slimdom.js/blob/align-with-spec/test/Document.tests.ts#L240, although it doesn't test the message.
This requires some restructuring in the way tests are transpiled, as nyc / istanbul seem to have trouble understanding the test/bin/test folder structure.
function isElement (node?: Node | null): boolean { | ||
return !!node && node.nodeType === Node.ELEMENT_NODE; | ||
} | ||
export default class Element extends Node implements ParentNode, NonDocumentTypeChildNode, ChildNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you consider overriding Function.name as a static property? This should make a dumped object slightly more readable. Also, this will make the asObject function simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript won't let me define a static name property, as it conflicts with the Function.name property. Instead, I've configured the babili minifier to leave the public type names unmangled and restored the old asObject implementation.
This pretty much rewrites the entire codebase to align its behavior with the DOM living standard (as of the current version, last updated 9 May 2017).
TODO before 2.0 release:
Nice to have: