Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use setTimeout instead of setImmediate #1482

Merged

Conversation

edi9999
Copy link
Collaborator

@edi9999 edi9999 commented Oct 15, 2019

Reasons for making this change

At one place in the library code, we use setImmediate.
However this function is not standard :

This method is not expected to become standard, and is only implemented by recent builds of Internet Explorer and Node.js 0.10+. It meets resistance both from Gecko (Firefox) and Webkit (Google/Apple).
Source

Since it is not standard, the setImmediate is transformed by babel to : var _setImmediate2 = _interopRequireDefault(require("@babel/runtime-corejs2/core-js/set-immediate")); which adds a lot of unneeded code to the build.

In my application tests, we use react-json-schema-form and are mocking the time with lolex, however, this "polyfil" by babel seems to not react to lolex.

setTimeout(function, 0) has the same behavior, it brakes no tests.

It would be great to release a patch version with those changes :-)

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

@edi9999 edi9999 requested review from glasserc and n1k0 October 15, 2019 10:15
@edi9999
Copy link
Collaborator Author

edi9999 commented Oct 15, 2019

@glasserc please let me know if you have any comments.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think for v2 we should remove setImmediate entirely (#1197), but it sounds good to add this for the patch version.

@edi9999 edi9999 merged commit 95263d1 into rjsf-team:1.x Oct 18, 2019
@edi9999
Copy link
Collaborator Author

edi9999 commented Dec 10, 2019

Hello @epicfaace , can you publish a patch version (1.8.1) which contains this fix ? It would be helpful for me :-)

@edi9999
Copy link
Collaborator Author

edi9999 commented Dec 10, 2019

up also @glasserc

1 similar comment
@edi9999
Copy link
Collaborator Author

edi9999 commented Dec 10, 2019

up also @glasserc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants