Patch promise constructor fix #25
Conversation
| // of Angular). | ||
| if (c === prop) { | ||
| return v | ||
| } |
There was a problem hiding this comment.
Do you think it could be risky to set a precedent of including specific fixes for different 3rd-party tools like Angular? Right now we have just this one, and perhaps it will stay like this, but I could envision a worst-case scenario where this file fills up with lots of different code paths for handling various different frameworks and libraries. We all know how the JavaScript ecosystem moves.
And in this case, it seems like Angular is moving away from these zones anyway. If the recommendation of the maintainers themselves is based on import order, maybe it's not so bad to follow that same pattern?
I don't have a strong feeling about this, though—just some rambly food for thought.
Would it make sense to also constrain this patch to w.Promise[p] by checking the name or target argument?
There was a problem hiding this comment.
I think we should be careful with fixes that are done for a particular framework - so I definitely agree with that.
In this case I think the fix doesn't hurt to have for non-Angular users too (it's possible other libaries will also overwrite the constructor). Angular is a pretty big library too (even if moving away from this feature.. some codebases are likely stuck in the past) - so in this case I think it makes sense.
Right now it is constrained to window.Promise.constructor because that's the only constructor we patch, which I think is not going to change for the foreseeable future.
There was a problem hiding this comment.
I'll add a comment to remind us if we add new constructor patches
Fixes #24.
Still putting it through its paces a bit to ensure this doesn't have unwanted side effects.