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

Replace setImmediate with setTimeout #201

Closed
wants to merge 1 commit into from

Conversation

NoelDeMartin
Copy link

It seems like setImmediate is not recommended for production code. I found about this when I realized it isn't available in worker threads.

One approach would be to create a helper method, but given that it isn't used a lot in the project I think the best approach is just to use setTimeout.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 687

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 686: 0.0%
Covered Lines: 1308
Relevant Lines: 1308

💛 - Coveralls

@RubenVerborgh
Copy link
Member

It seems like setImmediate is not recommended for production code.

For browser code. But N3.js also needs to work in Node, and setTimeout is slow.

Since browsers don't have setImmediate, so you can use a shim of your choice instead (including a setTimeout shim).

@NoelDeMartin
Copy link
Author

I see, I wasn't aware of that.

Having into account how simple the shim is, would you consider including it with the library? I don't mind having it in my projects, but it can save others some potential confusion.

@RubenVerborgh
Copy link
Member

I have no problem including it, but I'm not currently shipping with any browser configuration. So that would be a dependency. Maybe it can be a note in the README.

Not that where it really matters (https://github.com/rdfjs/N3.js/pull/201/files#diff-4437ff81da62d687c649e35228ccd752L1017), the shim is there already; the other one is just an edge case that will rarely happen multiple times per second, let alone at the point where it would make a tangible performance difference.

@NoelDeMartin
Copy link
Author

NoelDeMartin commented May 2, 2020

Ok I'll close this in favor of #202 to have a clean commit history.

@RubenVerborgh
Copy link
Member

For future reference: I instead moved to microtasks: ed3a787

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