coolaj86 / TextEncoderLite_tmp

Polyfill for the Encoding Living Standard's API
Apache License 2.0
30 stars 19 forks source link

Module.exports incorrect #8

Closed magyaatt closed 6 years ago

magyaatt commented 7 years ago

Why is only TextDecoderLite assigned to module.exports in text-encoder-lite.js? if(typeof module === "object" && module) { module.exports = TextDecoderLite }

Shouldn't that be more something like this? if(typeof module === "object" && module) { module.exports.TextDecoderLite = TextDecoderLite module.exports.TextEncoderLite = TextEncoderLite }

Ruffio commented 7 years ago

@magyaatt I understand your view. I think that most people only use the TextDecoderLite and that is why only that part has been exposed/exported. Personally I don't use Node myself, but with your suggested change, won't that break existing implementations? I think your point is valid and should be implemented. If change breaks existing implementations, then I think we should bump the version number. @coolaj86 what are your thought on this? @magyaatt why don't you come up with a PR for this?

coolaj86 commented 7 years ago

It can be done in non-breaking fashion:

module.exports.TextDecoderLite = TextDecoderLite;
module.exports.TextEncoderLite = TextEncoderLite;
TextDecoderLite.TextDecoderLite = TextDecoderLite;
TextDecoderLite.TextEncoderLite = TextEncoderLite;
module.exports = TextDecoderLite

Then it will work in browser and node systems as expected for all.

I believe that the docs should show this usage:

Node:

var TextDecoder = require('TextDecoderLite').TextDecoderLite;
var TextEncoder = require('TextDecoderLite').TextEncoderLite;

Browser:

var TextDecoder = window.TextDecoderLite;
var TextEncoder = window.TextEncoderLite;
zyrolasting commented 6 years ago

@coolaj86 Yeah, this drove me crazy. See #10

Ruffio commented 6 years ago

PR merged. This issue should be fixed now :-)