Skip to content

Commit

Permalink
refactor: provide default no-op implementation for events
Browse files Browse the repository at this point in the history
BREAKING CHANGE:

By default parsers now have a default no-op implementation for each
event it supports. This would break code that determines whether a
custom handler was added by checking whether there's any handler at
all. This removes the necessity for the parser implementation to check
whether there is a handler before calling it.

In the process of making this change, we've removed support for the
``on...`` properties on streams objects. Their existence was not
warranted by any standard API provided by Node. (``EventEmitter`` does
not have ``on...`` properties for events it supports, nor does
``Stream``.) Their existence was also undocumented. And their
functioning was awkward. For instance, with sax, this:

```
const s = sax.createStream();
const handler = () => console.log("moo");
s.on("cdata", handler);
console.log(s.oncdata === handler);
```

would print ``false``. If you examine ``s.oncdata`` you see it is glue
code instead of the handler assigned. This is just bizarre, so we
removed it.
  • Loading branch information
lddubeau committed Jul 2, 2018
1 parent 879d4a9 commit a94687f
Showing 1 changed file with 35 additions and 46 deletions.
81 changes: 35 additions & 46 deletions lib/saxes.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,6 @@ catch (ex) {
Stream = function FakeStream() {};
}

const streamWraps = exports.EVENTS.filter(ev => ev !== "error" && ev !== "end");

function isWhitespace(c) {
return c === " " || c === "\n" || c === "\r" || c === "\t";
}
Expand Down Expand Up @@ -164,9 +162,29 @@ class SAXParser {
if (this.trackPosition) {
this.position = this.line = this.column = 0;
}
this.emit("onready");
this.onready();
}

// We provide default no-op handlers.
/* eslint-disable class-methods-use-this */
ontext() {}
onprocessinginstruction() {}
ondoctype() {}
oncomment() {}
onopentagstart() {}
onattribute() {}
onopentag() {}
onclosetag() {}
onopencdata() {}
oncdata() {}
onclosecdata() {}
onerror() {}
onend() {}
onready() {}
onopennamespace() {}
onclosenamespace() {}
/* eslint-enable class-methods-use-this */

end() {
if (this.sawRoot && !this.closedRoot) {
this.fail("Unclosed root tag");
Expand All @@ -179,7 +197,7 @@ class SAXParser {
this.closeText();
this.c = "";
this.closed = true;
this.emit("onend");
this.onend();
this._init(this.opt);
return this;
}
Expand All @@ -193,7 +211,7 @@ class SAXParser {
}
er = new Error(er);
this.error = er;
this.emit("onerror", er);
this.onerror(er);
return this;
}

Expand Down Expand Up @@ -797,15 +815,9 @@ class SAXParser {
}
}

emit(event, data) {
if (this[event]) {
this[event](data);
}
}

closeText() {
if (this.textNode) {
this.emit("ontext", this.textNode);
this.ontext(this.textNode);
}
this.textNode = "";
}
Expand All @@ -814,7 +826,7 @@ class SAXParser {
if (this.textNode) {
this.closeText();
}
this.emit(nodeType, data);
this[nodeType](data);
}

attrib() {
Expand Down Expand Up @@ -1035,6 +1047,7 @@ class SAXParser {
}
}

const streamWraps = exports.EVENTS.filter(ev => ev !== "error");
class SAXStream extends Stream {
constructor(opt) {
super();
Expand All @@ -1043,9 +1056,15 @@ class SAXStream extends Stream {
this.writable = true;
this.readable = true;

this._parser.onend = () => {
this.emit("end");
};
this._decoder = null;

for (const ev of streamWraps) {
// Override the no-op defaults with handlers that emit on the
// stream.
this._parser[`on${ev}`] = (...args) => {
this.emit(ev, ...args);
};
}

this._parser.onerror = (er) => {
this.emit("error", er);
Expand All @@ -1054,26 +1073,6 @@ class SAXStream extends Stream {
// go ahead and clear error, so we can write again.
this._parser.error = null;
};

this._decoder = null;

for (const ev of streamWraps) {
Object.defineProperty(this, `on${ev}`, {
get() {
return this._parser[`on${ev}`];
},
set(h) {
if (!h) {
this.removeAllListeners(ev);
this._parser[`on${ev}`] = h;
return;
}
this.on(ev, h);
},
enumerable: true,
configurable: false,
});
}
}

write(data) {
Expand All @@ -1100,16 +1099,6 @@ class SAXStream extends Stream {
this._parser.end();
return true;
}

on(ev, handler) {
if (!this._parser[`on${ev}`] && streamWraps.indexOf(ev) !== -1) {
this._parser[`on${ev}`] = (...args) => {
this.emit(ev, ...args);
};
}

return Stream.prototype.on.call(this, ev, handler);
}
}

function createStream(opt) {
Expand Down

0 comments on commit a94687f

Please sign in to comment.