jindw / xmldom

A PURE JS W3C Standard based(XML DOM Level2 CORE) DOMParser and XMLSerializer.
Other
819 stars 265 forks source link

DOMParser fails when used in a promise in Node.js #222

Open luisespinal opened 7 years ago

luisespinal commented 7 years ago

DOMParser fails to bind to the proper "this" object when a DOMParser object is created outside of a promise, but used within a "thenable" block.

For instance.

const DOMParser = require('xmldom').DOMParser;
const parser= new DOMParser();
Promise.resolve(true)
                .then(readSomeFile)
                .then(parser.parseFromString)

Will cause the following error to occur.

(node:22420) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'domBuilder' of undefined
(node:22420) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

And that is because the "this" object has changed from where the parser was created and from the moment the promise is being evaluated. The error is on the third line below (in dom-parser.js)

DOMParser.prototype.parseFromString = function(source,mimeType){
    var options = this.options;
    var sax =  new XMLReader();
    var domBuilder = options.domBuilder || new DOMHandler();//contentHandler and LexicalHandler

The options variable is going to be undefined (because the "this" object referring to the context being executed in the current "thenable" block does not have such a property. Then when we try to create domBuilder, it inevitably breaks.

  1. One solution is to lazy evaluate construction of the parser till the last moment (not always possible)
const DOMParser = require('xmldom').DOMParser;
Promise.resolve(true)
                .then(readSomeFile)
                .then((x)=>{return (new DOMParser()).parseFromString(x)}) // meh, ok sometimes
  1. Or, if you must instantiate the parser outside of the thenable blocks, to lazily and explicitly bind to it
const DOMParser = require('xmldom').DOMParser;
const parser= new DOMParser();
Promise.resolve(true)
                .then(readSomeFile)
                .then(function(){return DOMParser.parseFromString.apply(parser,arguments);}) // UGLY!!!!
  1. Better yet, to bind parseFromString at the moment of construction in dom-parser.js, changing the constructor from this:
function DOMParser(options){
    this.options = options ||{locator:{}};
}

To this:

function DOMParser(options){
    this.options = options ||{locator:{}};
    var me = this;
    this.parseFromString = function(){ 
        return DOMParser.prototype.parseFromString.apply(me,arguments); 
    };
}

I've tested all changes, and my preference would be the last one.

Until (and if) such a change is incorporated, the workaround I'm using is number one and two above, depending on context.

markstos commented 7 years ago

I would expect the behavior you experience. It's common to need to explicitly bind a method to object when passing the method as a function. Rather than patching this code, I would expect a bound function to passed into the promise:

    parser.parseFromString.bind(parser)