sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
802 stars 105 forks source link

Fixing issue #319 - preventing Rollup eval error #329

Closed itayperry closed 4 years ago

itayperry commented 4 years ago

Purpose

This is the first step (out of 3) in solving issue #319. The use of eval() will eventually not be canceled, and this pull request tricks Rollup to not throw an error when eval() is used.

codecov[bot] commented 4 years ago

Codecov Report

Merging #329 into master will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage   92.75%   92.77%   +0.02%     
==========================================
  Files           1        1              
  Lines         552      554       +2     
==========================================
+ Hits          512      514       +2     
  Misses         40       40              
Flag Coverage Δ
#unit 92.77% <100.00%> (+0.02%) :arrow_up:
Impacted Files Coverage Δ
src/fake-timers-src.js 92.77% <100.00%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1f83b99...a211987. Read the comment docs.

benjamingr commented 4 years ago

Hey, thanks, can you please add a test that eval runs on global scope?

itayperry commented 4 years ago

I don't really know how to test it by scope, I would be happy to get a piece of advice. There are a few tests that check if the eval() generally works, but I'm not sure if they really check its scope. One of them is: https://github.com/sinonjs/fake-timers/blob/89e9e9b58c12fdf3301067dee6282ca829c48c56/test/fake-timers-test.js#L323

benjamingr commented 4 years ago

Define a local variable, make a setTimeout, assert that it is not defines in the timeout (by calling tick)

itayperry commented 4 years ago

In https://github.com/sinonjs/fake-timers/blob/89e9e9b58c12fdf3301067dee6282ca829c48c56/src/fake-timers-src.js#L414 the function I added:

var eval2 = eval;
(function() {
    eval2(timer.func);
})();

gets a variable that will always be global to it.

I understand that if I do something like:

var eval2 = eval;
(function() {
    var x = 5; 
    eval2('x') 
    eval2(timer.func);
})();

It will throw a reference error saying that x is undefined, but in the way that the function is built now there's no way (from the outside through setTimeout) to insert a variable that will be local in this function.

I've tried doing something like setTimeout('var x = 5', 10) but this triggers a leak error. I'm sorry for sending so many questions @benjamingr , but I wanna solve this issue perfectly :)

benjamingr commented 4 years ago

@fatso83 how is the baby situation? Any chance to get a release? There are some pending changes and I'm scared to release myself because I haven't released fake-timers yet.