-
Notifications
You must be signed in to change notification settings - Fork 84
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: Update DOMImplementation
according to recent specs
#210
Conversation
- deprecate `hasFeature` and simplify implementation according to the latest spec - `createDocumentType` sets `publicId` and `systemId` to an empty string instead of `undefined` - `createDocument` sets `docType` to `null` instead of `undefined` - export `DocumentType`, `Element`, `Node` and `NodeList` to verify instanceof in tests
DOMImplementation
against (latest) specs
function DOMImplementation(/* Object */ features) { | ||
this._features = {}; | ||
if (features) { | ||
for (var feature in features) { | ||
this._features = features[feature]; | ||
} | ||
} | ||
}; |
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.
The previous implementation left the definition of features and versions that are supported to the code that calls the constructor, which doesn't make any sense at all. This was one more reason why I decided to not keep or fix it, but to implement it as it is specified in the living standard.
DOMImplementation
against (latest) specsDOMImplementation
according to recent specs
Element, | ||
Node, | ||
NodeList, | ||
} = require('../../lib/dom') |
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.
I would favor that we export and use these from lib/index.js
, as I proposed in PR #233.
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.
Does it mean you want to land #233 first? Since lib/index.js
doesn't exist on master yet.
It currently already has a conflict and I raised two questions there.
If you think it's important to solve this before anything else, let's focus on that issue.
If not I would prefer to leave this as is in this PR, land it and continue to solve that issue in the related PR.
- deprecate `hasFeature` and simplify implementation according to the latest spec - `createDocumentType` sets `publicId` and `systemId` to an empty string instead of `undefined` - `createDocument` sets `docType` to `null` instead of `undefined` - export `DocumentType`, `Element`, `Node` and `NodeList` to verify `instanceof` in tests - update doc comments with links to specs and type information
hasFeature
and simplify implementation according to the latest speccreateDocumentType
setspublicId
andsystemId
to an empty string instead ofundefined
createDocument
setsdocType
tonull
instead ofundefined
DocumentType
,Element
,Node
andNodeList
to verifyinstanceof
in testsThis PR is part of the preparation for solving #203