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

fix(messaging): Remove non standard functions (setImmediate, clearImm… #25

Closed
wants to merge 1 commit into from

Conversation

GordonSmith
Copy link
Contributor

…ediate)

Remove non standard set/clear Immediate and replace them with an animationFrame polyfill

Signed-off-by: Gordon Smith [email protected]

…ediate)

Remove non standard set/clear Immediate and replace them with an animationFrame polyfill

Signed-off-by: Gordon Smith <[email protected]>
@GordonSmith
Copy link
Contributor Author

Depending on what browsers you support, there may be no need for the polyfill at all (IOW you could assume that requiestAnimationFrame always exists)?

@afshin
Copy link
Member

afshin commented Dec 10, 2019

The @lumino/messaging library is meant to support node as well. It is not browser-only, so the logic ought to remain as is.

@afshin afshin closed this Dec 10, 2019
@GordonSmith
Copy link
Contributor Author

@afshin just to clarify, your closing because:

  • All browsers have adequate requestAnimationFrame support
  • If requestAnimationFrame is absent, your assuming its a NodeJS environment and setImmediate is known to exist
    ?

@afshin
Copy link
Member

afshin commented Dec 10, 2019

Yes, the browsers Lumino supports have requestAnimationFrame and the only other environment we currently expect this library to run in is node, which has setImmediate defined.

@GordonSmith
Copy link
Contributor Author

Ok - works for me (FYI you will start to get TS syntax errors in the future - but a ts-ignore and suitable comment should be fine...)

@GordonSmith GordonSmith deleted the STANDARDS branch December 10, 2019 10:26
@afshin
Copy link
Member

afshin commented Dec 10, 2019

Can we add "node" to the lib array in tsconfig.json to resolve that when it arises?

@GordonSmith
Copy link
Contributor Author

GordonSmith commented Dec 10, 2019

Probably - but not with the current TS compiler version. (I will test later today - slowly working through my forks diffs).
FWIW you should avoid upgrading to TS compiler 3.7.x due to: microsoft/TypeScript#33939

@afshin
Copy link
Member

afshin commented Dec 10, 2019

Cool, thank you!

@GordonSmith
Copy link
Contributor Author

Looks like there isn't a specific ts lib option for this. We can continue this discussion when I issue a PR for TSC 3.6.4

@afshin
Copy link
Member

afshin commented Dec 10, 2019

Cheers!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants