eligrey / Blob.js

An HTML5 Blob implementation
Other
1.15k stars 605 forks source link

fix rollup error #56

Closed Uzlopak closed 6 years ago

eligrey commented 6 years ago

Could you go into more detail as to what rollup error this fixes and why you're using Function(...)() instead of eval(...)?

Uzlopak commented 6 years ago

Hi,

Problem is that when we use rollup, then rollup tries to replace the this-object to undefined.

I described it here https://github.com/MrRio/jsPDF/issues/1533

Or in short: The this keyword is equivalent to undefined at the top level of an ES (6?) module, and will be rewritten by rollup. https://github.com/rollup/rollup/issues/847

It would be great if you change it, so that it will result in less problems. Atleast I hope it :).

Why not eval you ask. Good question. It works and I think I read the usage of "Function()" as a recommendated solution. But this is your decision. :)

Uzlopak commented 6 years ago

Btw:

This is identical to https://github.com/eligrey/FileSaver.js/pull/387

eligrey commented 6 years ago

I'm actually unsure what package manager even uses this.content. It was added by someone else, and you can entirely drop the two lines. If you update your commit to simply remove || this.content || this I'll go ahead and merge this.

Uzlopak commented 6 years ago

I removed this.content() and this. I kept global, because global is used by nodejs. Hope this is how you can accept it :).

Uzlopak commented 6 years ago

I realize now: In your FileSaver.js-Project there is a comment:

// `self` is undefined in Firefox for Android content script context
// while `this` is nsIContentFrameMessageManager
// with an attribute `content` that corresponds to the window

Maybe this.content is something you need for writing plugins for Firefox?

Uzlopak commented 6 years ago

Do you still want this changes? Depending on your decision I would modify Blob.js and FileSaver.js

eligrey commented 6 years ago

Seems like for best compatibility we should keep one of the this references (the one that wouldn't cause any ReferenceErrors). Please put || this back at the end of the statement and then increment the version number at the top of the script to 2018-01-12, and then I will merge.

Uzlopak commented 6 years ago

Hi eli,

I double-checked it: Function('return this')() equals this

So it would be redundant to have a last || this

Uzlopak commented 6 years ago

So you want that I remove the this.content reference? Maybe some people use it? Waiting for your instructions.

eligrey commented 6 years ago

I'm a asking for to both Function() calls to be removed. || this will suffice and won't be redundant.

Uzlopak commented 6 years ago

Thank you. How about FileSaver.js? I just added now the global-scope. Do you want me to remove this.content there too?