Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add JSTimer implementation with tests #46

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

Merged
MaxDesiatov merged 9 commits into master from jstimer
Sep 15, 2020
Merged

Add JSTimer implementation with tests #46

MaxDesiatov merged 9 commits into master from jstimer
Sep 15, 2020

Conversation

@MaxDesiatov
Copy link
Member

@MaxDesiatov MaxDesiatov commented Sep 13, 2020

Depends on #45. This is also a prerequisite for a future JSPromise PR with tests.

I intentionally didn't match the JS API in this PR, as a special care is needed to hold a reference to the timer closure and to call .release() on it. Here, a user is supposed to hold a reference to a JSTimer instance for it to stay valid. The API is also intentionally simple, the timer is started right away, and the only way to invalidate the timer is to bring its reference count to zero.

If you see a better way to manage closures passed to setTimeout and setInterval, I'd be happy to consider that.

Also, Node.js and browser APIs are slightly different. setTimeout/setInterval return an object in Node.js, while browsers return a number. Fortunately, clearTimeout and clearInterval take corresponding types as their arguments, and we can store either as JSValue, so we can treat both cases uniformly.

kateinoigakukun reacted with thumbs up emoji
Copy link
Member

j-f1 commented Sep 13, 2020

  • I think it would make sense to use the standard TimeInterval type here and convert it to the appropriate millisecond value to better match other Swift APIs
  • the API is pretty verbose. What do you think of this?
public final class JSTimer {
 public init(_ after: TimeInterval, _ callback: @escaping () -> ())
 public static repeating(_ interval: TimeInterval, _ callback: @escaping () -> ())
}

Copy link
Member

@j-f1

I think it would make sense to use the standard TimeInterval type here and convert it to the appropriate millisecond value to better match other Swift APIs

It's reasonable. But unfortunately, TimeInterval is a type defined in Foundation and I don't want JavaScriptKit to depend on Foundation.

j-f1 and MaxDesiatov reacted with thumbs up emoji

Base automatically changed from jsdate to master September 14, 2020 09:48
@MaxDesiatov MaxDesiatov marked this pull request as ready for review September 14, 2020 09:50
Copy link
Member Author

MaxDesiatov commented Sep 14, 2020
edited
Loading

@j-f1

the API is pretty verbose. What do you think of this?

public final class JSTimer {
 public init(_ after: TimeInterval, _ callback: @escaping () -> ())
 public static repeating(_ interval: TimeInterval, _ callback: @escaping () -> ())
}

I'm not sure this would work. repeating still has to delegate to some initializer, but there isn't one in this case that would call setInterval, if we assume that the only present init calls setTimeout. Then it would have to look something like:

public final class JSTimer {
 public init(millisecondsDelay: Double, isRepeating: Bool = false, callback: @escaping () -> ())
 public static func repeating(millisecondsDelay: Double, callback: @escaping () -> ())
}

Maybe we could just add a default false value on isRepeating without adding static func repeating? WDYT?

j-f1 reacted with thumbs up emoji

@MaxDesiatov MaxDesiatov merged commit 0ac8b37 into master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kateinoigakukun kateinoigakukun kateinoigakukun approved these changes

@j-f1 j-f1 Awaiting requested review from j-f1

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /