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

Add setImmediate #40

Merged
merged 1 commit into from
May 16, 2018
Merged

Add setImmediate #40

merged 1 commit into from
May 16, 2018

Conversation

jscissr
Copy link
Contributor

@jscissr jscissr commented Mar 3, 2015

setTimeout, setInterval, setImmediate and their clear counterparts are globals in Node.
set/clearImmediate are not available in all browsers, so I added them here.

setTimeout, setInterval, setImmediate and their clear counterparts are
globals in Node. Because set/clearImmediate are not available in all
browsers, adding them here.
@jscissr
Copy link
Contributor Author

jscissr commented May 14, 2015

Can you take a look at it, please?

@jscissr
Copy link
Contributor Author

jscissr commented May 26, 2015

This pull request is still unanswered. @substack ?

@jscissr
Copy link
Contributor Author

jscissr commented Jul 20, 2015

ping @substack

@zertosh
Copy link
Member

zertosh commented Jul 26, 2015

@jscissr What's the status of setImmediate and clearImmediate as web standards?

@ghost
Copy link

ghost commented Jul 26, 2015

@zertosh there has been discussion for years but nobody except for IE and node care. I'm a bit split on whether insert-module-globals should detect this case, since people can always add their own polyfills and node 0.8 didn't have setImmediate either, it showed up in 0.10.

@zertosh
Copy link
Member

zertosh commented Jul 26, 2015

@substack In that case, it should be left for users to polyfill then.

@jscissr Alternatively, you use browserify's insertGlobalVars to get the same effect as your PR:

var b = browserify({
  /* more options */
  insertGlobalVars: {
    setImmediate: function () {
        return 'require("timers").setImmediate';
    },
    clearImmediate: function () {
        return 'require("timers").clearImmediate';
    }
  }
});

@jscissr
Copy link
Contributor Author

jscissr commented Jul 27, 2015

I don't need this PR for myself anymore @zertosh. When I opened the PR I experimented with using express in a Chrome App using browserify, and express uses the setImmediate global (which is not available in Chrome). There were other problems so I didn't use express in the end, but there are surely other packages that use it.

Both node and io.js support it, however they don't document it (node, io.js). I didn't find any deprecations, it's simply an undocumented feature.

So do you think that instead of this PR, the packages that use the globals shouldn't use them? Or should the people who use these packages with browserify add the snippet in the above comment?

@zertosh
Copy link
Member

zertosh commented Aug 5, 2015

So do you think that instead of this PR, the packages that use the globals shouldn't use them?

There are legitimate uses for setImmediate, so that's on you whether you want to use them or not.

Or should the people who use these packages with browserify add the snippet in the above comment?

Yeah, the snippet is straight forward enough to drop in if you need it.

@jscissr
Copy link
Contributor Author

jscissr commented Aug 5, 2015

So do you think that instead of this PR, the packages that use the globals shouldn't use them?

There are legitimate uses for setImmediate, so that's on you whether you want to use them or not.

I didn't mean whether I should use setImmediate or not, but rather which of these three options you'd prefer:

  1. Packages should use var timers = require('timers'); ... timers.setImmediate(...); instead of just setImmediate(...); which depends on an undocumented global.
  2. Whenever someone wants to use such a package in a browserify project, he/she has to consult the documentation of browserify or find this pull request, and manually add the snippet.
  3. This PR is merged.

I would prefer 3. because it's the easiest. I thought the idea of browserify is to have a node environment in browsers, and I don't see why setImmediate shouldn't be included. I understand that it is not a web standard, but people are using it; and e.g. Buffer is not a web standard too, but is supported by insert-module-globals.

@mattdesl
Copy link

I feel +1 on this since I have seen multiple node modules that would work fine in browserify if setImmediate was polyfilled.

Same reason process, __dirname, global, etc are shimmed: so that we have better compatibility with node/npm modules.

related: avajs/ava#24

@mattdesl
Copy link

Actually, it is a bit of a tricky issue. 😕 It would not be a good thing to add non-standard stuff to browserify.

Note: 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).

https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate

EDIT: But it does seem pretty standard in Node going forward, so maybe that is enough reason to merge this PR.

@mattdesl mattdesl mentioned this pull request Aug 28, 2015
@sindresorhus
Copy link

👍 Should be merged. Not doing so means users will have to manually add a polyfill if some deep sub-dependency makes use of setImmediate, which is often the case. This would make the life easier for a lot of Browserify users.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This should definitely be merged; specifically I want to note that it doesn't shim anything - ie, it doesn't install setImmediate in the global browser environment - it just provides a replacement for code written in a node style that expects it to work.

@goto-bus-stop
Copy link
Member

yes, agreed

(timers-browserify@2 does use a global shim, but browserify still ships with @1 by default. the newer timers versions use setimmediate which only has a global polyfill.)

@goto-bus-stop goto-bus-stop merged commit bc17071 into browserify:master May 16, 2018
@jscissr jscissr deleted the immediate branch May 17, 2018 22:25
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.

6 participants