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

[TIMOB-26184] iOS: Allow modification of global timer functions #10155

Merged
merged 2 commits into from Jul 12, 2018

Conversation

janvennemann
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-26184

Optional Description:
Allow modifying some of our global function like setTimeout or setInterval. This is possible in other JS environments too and required for using Jasmin for example.

Note that this is already possible on Android.

@build
Copy link
Contributor

build commented Jul 5, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

Code change LGTM. Please add a unit test for modifying the methods as part of our mocha-test-suite. Thanks!

@hansemannn hansemannn added this to the 7.4.0 milestone Jul 5, 2018
@hansemannn hansemannn changed the title [TIMOB-26184] Allow modification of global timer functions [TIMOB-26184] iOS: Allow modification of global timer functions Jul 5, 2018
@sgtcoolguy
Copy link
Contributor

Just confirmed that on Node the setTimeout function is writable/enumerable/configurable, which is what this change should do... 👍

You could write a test that overrode the impl and then tried to set it back, but I worry that might interfere with mocha actually properly running the tests. It's probably sufficient to call Object.getOwnPropertyDescriptor(global, 'setTimeout') and verify that it's enuemrable/writable/configurable.

@ewanharris
Copy link
Collaborator

Just to double check my reading, this will make the 4 timeout/interval based functions, alert, L and require overwrite-able. Will that the same across all platforms? (not saying we shouldn't, just making sure it's the expected)

@janvennemann
Copy link
Contributor Author

@ewanharris Yup, that is exactly the intention of this PR. On Android this is already the case, although i'm not familiar enough with the V8 sources to say where it is configured there.

I'll write the appropriate unit tests after doc day to see if it is working as intended on both Android and iOS.

@sgtcoolguy sgtcoolguy merged commit 95c0b53 into tidev:master Jul 12, 2018
@hansemannn hansemannn modified the milestones: 7.4.0, 7.5.0 Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants