NativeScript / ios-jsc

NativeScript for iOS using JavaScriptCore
http://docs.nativescript.org/runtimes/ios
Apache License 2.0
299 stars 59 forks source link

Missing __requireOverride #679

Closed NathanaelA closed 7 years ago

NathanaelA commented 8 years ago

The android team added a __requireOverride command to the engine to allow a process to intercept the require before it finishes. However, it would be nice if this was also supported on iOS.

Link to Android implementation: https://github.com/NativeScript/android-runtime/blob/b32ab11f44cc85ed498453d21bc43ef97ea262c7/runtime/src/main/jni/Module.cpp

I use this feature to allow hot-patching content on android in this updater-android

jasssonpet commented 8 years ago

Hey, @NathanaelA,

The require implementation of the iOS runtime is hooked with the ES6 module loader of the JavaScriptCore engine (source). You can try exposing and playing with the Loader APIs:

require('./test'); // test.js evaluated
Loader.resolve('./test').then(function(absolutePath) {
    Loader.registry.delete(absolutePath); // delete the module cache
    require('./test'); // test.js evaluated again and cached
});

To enable it, you have to set the following environment variable in the app scheme: JSC_exposeInternalModuleLoader=YES. Additionally, to aid debugging you can also set JSC_dumpModuleLoadingState=YES.

Let us know if this works for you or we will try to figure something else out.

P.S. Please note that JSC_ options are not currently working in the @next branch (https://github.com/NativeScript/webkit/pull/12).

NathanaelA commented 8 years ago

@jasssonpet I actually just monkey patched the require statement at runtime. So I have a workaround. ;-) The only reason I reported this was because of the inconsistency. These inconsistencies drive me nuts as I'm having to put a lot of code into my apps and plugins if (global.android) {x} else {y} And I shouldn't have to do that with the environment...

NathanaelA commented 7 years ago

Not sure who is dealing with this now that Jason is just a contributor, but I found a interesting issue that I'm not sure how to cover on iOS, that the android __requireOverride does handle this properly.

Take this for example: app/screen1/file1.js app/screen1/file2.js

In file1.js I can do require('./file2.js'); and the require command being built in will know I'm in the screen1 folder. However, if I monkey-patch the "require" command on iOS; the ./ is actually the path where my file that contains my monkey patch file resides, from all my tests.

What is the way I can determine the correct path for ./? In the android environment, the __requireOverride the second variable covers it, the first is the name a require is looking for; the second is the path of where the file is being requested from. So I can easily determine that ./ = the path requested from. ;-)

Any idea on how to solve this one, or some way I can get the priority of this feature added bumped up (even if I do it myself?)

ivanbuhov commented 7 years ago

@NathanaelA I am not sure how exactly you are monkey-patching the require function but I guess that the file containing the monkey-patch looks like this:

module.exports = function customRequire(path) {
  console.log("Custom logic before requiring " + path);
  require(path);
  console.log("Custom logic after requiring " + path);
}

and it is used in other modules like this:

var customRequire = require("./custom-require");
var myModule = customRequire("./my-module"); // this will be resolved relatively to the custom-require.js file

If this is the case, you can make it work as expected if your customRequire() function receives an underlying require function as a parameter:

module.exports = function customRequireFactory(requireFunc) {
  return function customRequire(path) {
    console.log("Custom logic before requiring " + path);
    requireFunc(path);
    console.log("Custom logic after requiring " + path);
  }
}

and it is used in other modules like this:

var customRequire = require('./custom-require')(require);
var myModule = customRequire("./my-module"); // this will be resolved relatively to the current file because the current file's require function is used in custom-require.js to load the module

Let me know if this is not working for you.

NathanaelA commented 7 years ago

Here is the issue, I've written a generic app updater plugin; I can update a NS app w/o sending it to the stores, the app can automatically downloads an update and then applies it. To do this seamlessly this requires me to override the "require" statement as I need to redirect to the newer versions of the files IF they exist. So I have to take the file they are looking for and then see if the new version exists in an updated folder.

So, actually this is what I'm doing is something like this on iOS.

const oldRequire = global.require;
global.require = function(fileName) {
   // code to see if this file should be loaded from an updated location, if so load it
  if (needsToOverride) {
       return oldRequire.apply(this, calcuatedFileName);
   } else {
       return oldRequire.apply(this,fileName);
   }
}

So lets take some real code:

var stackLayout = require("ui/layouts/stack-layout/stack-layout"); The first line of code it does is: var common = require("./stack-layout-common");

So now when it hits my code I cannot determine with "./" is because the __dirname for where my "require" lives is different than the __dirname of the stack-layout code. And of course there doesn't seem to be anything that can tell me where "./" or "../" is located.

On android it has the cool __requireOverride command which eliminates me having to monkey patch the require command, AND requireOverride has the following parameters:

global.__requireOverride = function(requireFileName, requireFromPath) {
   // Calculate real path to load code from based on requireFromPath, and requireFileName
   if (needToOverride) return require(pathToNewVersion);
   else return false;
}

That functionality it perfect, since it passes me in both the file required ("./stack-layout-common") and it passes me in "/data/data/appname/files/app/tns-modules/ui/layouts/stack-layout" and so I can automatically calculate "." and ".." paths and then create the actual path to the file.

Now at this point, I'm just going to have to tell people that relative path's are NOT supported since the iOS engine doesn't support it. But I would like to get relative path's working asap; as I know a lot of people (including myself prefer to) use things like this: "var x = require("../../support/animation") from a folder like /app/views/coolView to require a file in /app/support/

ivanbuhov commented 7 years ago

@NathanaelA Thanks for the provided details. I understand what you are trying to achieve. I recommend using the solution described in my previous comment. It should work equally good for both iOS and Android, it works with any path (relative and absolute), it doesn't use platform-specific functionality and it doesn't hide the global require function giving you the possibility to use both the original require and the monkey-patched require functions.

NathanaelA commented 7 years ago

@ivanbuhov - Uhm, here is the issue and why I have to override "require" -- None of this code beyond my updater is under my control. I can't really go through all of TNS core modules and replace everywhere you use "require" and put in my "customrequire". ;-) Which is needed for me to overwrite any navigation commands. Nor can I tell all the customers to swap out their TS import x from 'y'; and break all their code and intellisense.

NathanaelA commented 7 years ago

@ivanbuhov - I have ran into another issue with v2.5.0 -- it appears that the require is now marked read only. I don't know if you fixed it so that readonly now works or it was just recently marked as read only; but this totally breaks my code on iOS and so now my updater project cannot NOT work on iOS. Without the __requireOverride or the ability to override the require statement, I am unable to capture loading of any JS and redirect that loading to the new location.

https://github.com/NativeScript/ios-runtime/blob/master/src/NativeScript/GlobalObject.moduleLoader.mm#L439

Can we get the "readonly" removed from this -- for the next release as this is a change where I was able to do this in 2.3.x fine. This command should NOT be marked read only as if I want to override it; I should have the ability to override it. Especially if you aren't going to bring the __requireOverride that the android runtime has to the iOS runtime..

Again, I need to be able to fully control the require so that I can redirect any var x = require('x'); to go to the updated x file. Android side works 100%, iOS now totally broken.

ivanbuhov commented 7 years ago

@NathanaelA In a NativeScript module you have access to 3 require functions: global.require, module.require and require. The first one is a read-only property of the global object and it is accessible in all modules. The global require function always resolves module identifiers relatively to the root app folder, no matter where it is used. The second and third are actually the one and the same function object. This object is module-specific, so it is different for every module. It is first added as a read-write property to the module object and then passed to the module wrapping function as a parameter. This means that both module.require and require functions can be overridden, but the override will take effect in the current module only. The above should be true for version before v2.5 and after v2.5.

Additionally, if you were able to override global.require this will not give you much power, because the module specific require doesn't call global.require in its implementation. Actually both require and global.require call one and the same native function. This means that you need to override the module-specific require which by definition must be done for every single module.

Since our require is hooked with the ES6 module loader, instead of providing our custom hooks like __requireOverride I am more fan of exposing the hooks defined by the Loader "spec".

NathanaelA commented 7 years ago

@ivanbuhov - I see what you mean; I marked the require as not-read only and overrode it and it worked for some files; but others it called into the native code bypassing my require code. :( I could has sworn this worked in 2.3; was this something that you guys changed in 2.4/2.5?

What type of ETA do you think on doing either the __requireOverride or the ES6 module loader hooks, like the resolve hook? Or do you have another way that I can proceed so that I can bring this functionality to iOS; because at this time Android is fully supported but iOS is broken...

NathanaelA commented 7 years ago

@ivanbuhov - Ok, I have put in a put a couple patches into place into the runtimes to allow me to do this. However, I need to know if this is something the :NS: team is willing to accept or if you want to wait until you can put in proper module loader hooks.

If you want to wait, I don't have a problem telling people that my runtimes are better, so they need to use mine with the --frameworkPath. ;-)

Here is what I did.

  1. Disabled the readonly on both require locations in the globalmodules. Really only the global.require needs it; but the other one was just in case. No real good reason I can think of to have them marked as read only anyways...

  2. I Slightly modified the moduleFunctionSource from:

    moduleFunctionSource.append("{function anonymous(require, module, exports, __dirname, __filename) {");
    moduleFunctionSource.append(source);
    moduleFunctionSource.append("\n}}");

to

moduleFunctionSource.append("{function anonymous(requireSrc, module, exports, __dirname, __filename) {");
moduleFunctionSource.append("var require=requireSrc; if (typeof global.__getRequire === 'function') {  require = __getRequire(requireSrc, __dirname); }");
moduleFunctionSource.append(source);
moduleFunctionSource.append("\n}}");

The speed hit is negligible; in fact it is faster than several of the hacks that were put into place for NG2 support in the tns runtimes.

If you haven't setup a __getRequire function then the path works almost identically. require is set to requireSrc. If you do have a getRequire function then the require gets set to whatever overriding require function that the user assigned which can determine the path properly because of the dirname passed into it. ;-)

Now I realize this is NOT as nice as a full loader module code; but since I have no time table on it coming; and my past experience has been that features like this a very low priority -- would you be willing to take a patch that does this so that I do have a workable solution that can be built into the existing runtimes and will work for anyone attempting to override the require system.

I had originally looked at doing the __requireOverride but it appears to be slower to do the work to check to see if it exists, then call BACK into the JS runtime to run it... So this is my current simple solution until module loaders can be built into the iOS runtimes.

And look it WORKS great: ios-updater

This simple example shows how the updater can automatically use the new versions of the files after it downloads it. (xml, js & css) -- In my demo case; there are three versions. Yellow (not shown in gif but it is the app's original main-page.js/.xml/.css), and two updates Blue, and Red which are downloaded from the server. When you click the update button it passes the current version "yellow", "blue", "red" to the server, the server goes and sends them the other version. Then after the app downloads, checks and extracts the new page it calls the callback letting the app know it has an update. Which then for the demo just calls the built in "frame.reloadPage()". And the new require and xml/css loading code automatically maps the files back to either the original app locations or the new version locations depending on if a new version exists. And bam, we have live updating and the app you can replace any file but the initial bootstrap app.js file.

ivanbuhov commented 7 years ago

@NathanaelA

I marked the require as not-read only and overrode it and it worked for some files; but others it called into the native code bypassing my require code. :( I could has sworn this worked in 2.3; was this something that you guys changed in 2.4/2.5?

I can't say what's the problem (look at number 6 in the list below). Actually the global require should not be called at all, so overriding it should have no effect. Every module is using its local require function but if you send the code I'll be able to give more concrete answer.

What type of ETA do you think on doing either the __requireOverride or the ES6 module loader hooks, like the resolve hook? Or do you have another way that I can proceed so that I can bring this functionality to iOS; because at this time Android is fully supported but iOS is broken...

The __requireOverride solution is easy and it can be implemented in a day or two but If we decide to do the ES6 module loader hooks instead - it will definitely take more time and it will not be ready for 3.0. Also it is matter of priority which timeframe the issue will be planned for, so it's hard to give ETAs before we have decided which approach to take and what is the priority of the issue.

Let's first agree on which is the right approach from a technical perspective and after that we can prioritize, estimate and plan the effort. Additionally, if we have once agreed on the solution, someone from the community may be interested in helping the core team by implementing it.

I have some concerns about your solution. I will list them here but if you have a ready-to-commit branch you can make a PR just to make the code public. It will be easier for me to discuss a feature after inspecting the code.

  1. I can't understand why global.require must be overridden. Given that you already have moduleFunctionSource patched why you need to override the global require function? Maybe I can't understand the proposal in details, so providing the code in the form of a branch/PR will make the discussion easier.

  2. You have to manually add the module to the registry cache or have a custom exports caching mechanism. Providing ES6 module loader hooks will take care of that automatically.

  3. Since the iOS Runtime supports ES6 modules which use the module loader directly (without calling the require function) the global.__getRequire hook will never be called in ES6 modules context. This is not an issue with ES6 module loader hooks.

  4. We must patch also the module.require function because a module can be required like this: var m = module.require('m'); instead of var m = require('m');.

  5. It is not standardized. For example, polluting the module function scope with additional variable requireSrc which is not standardized in the commonJS spec should be avoided. The chance this variable to break a regular commonJS module is very low but still exists. IMO, we must avoid such changes.

  6. It is obvious that the require hook will not be called for the entry module but additionally it will not be called for the require functions in the entry module, too, because the entry module is not loaded by calling the require function but it is loaded by calling the module loader API directly and the newly introduced logic in the module wrapper function will not be executed.

These are some concerns that I have on the proposed global.__getRequire approach. Most of them are valid for the __requireOverride approach, too. To be honest I see the following benefits of the __requireOverride approach against the ES6 module loader hooks:

  1. Less time to implement.

  2. The Android runtime has it.

After measuring the pros and cons my opinion is that we have to put all of the effort in implementing ES6 module loader hooks, instead of providing (maybe temporary) solutions with such known issues. If you agree that ES6 module loader hooks is the right approach I will add the issue to the backlog for further planning/estimation. If you want to help with the implementation - you are more than welcome :). But what is most interesting for me is your technical opinion on the topic __requireOverride vs global.__getRequire vs ES6 module loader hooks. Can you see something I have missed in the lists of pros and cons?

NathanaelA commented 7 years ago

Some design information; The updater project allows you to send only new versions of some files; any files that don't exist in the "new" folder are automatically loaded from the original directory. This way it allows incremental updates. So the logic for each require is does it exist in the new version directory, if not, then load it from the original location.

I currently have two handlers on iOS. global.require & global.__getRequire Both of these have very similar code; but the global.__getRequire is called only by the modules require, the global.require is called at least for the main-page.

On Android I have a single handler; __requireOverride it gives me access to everything I need.

I understand what you are saying, but I can tell you the global.require is being called; not sure under which circumstances as that was my first try was just overriding that; and I am getting logging from my "new" require function. (I am logging what file was being requested and if it ended up overriding the path because a new version existed). So when ever the global.require overrode the path to the "new" version of the file (and it loaded it), any files that that file required which didn't exist in the "new" version would break because the modules require didn't know to check the old path. So this was how I knew my solution wasn't complete on iOS. ;-)


  1. No idea, to be honest -- I haven't tried my code with just the module.require. I do know my global.require function is logging when it gets called, and it is a separate function than the __getRequire function, so the only way it could be logging is if it is called at some point.

  2. I'm not understanding your second point; I have to do what? My __getRequire all it does is figure out if the path is correct, if it isn't then it calls the original require with the correct path. If it is then it calls the original require with the original path. -- I'm letting the original and real require (i.e. all the iOS framework code) do its work normally, all I'm doing is intercepting the request first and verifying the location it needs to pull the JS file from, and in some cases changing the path.

  3. Sorry, you lost me on 3. Not sure if it is relevant to what I'm doing. ;-D

  4. Does module.require actually work properly in NS on both iOS and Android or are you talking potentially with ES6 loader stuff? If this isn't cross platform, then I wouldn't even worry about it as I can't think of a reason I would use module.require over just require if my code was going to break on Android....

  5. I understand the issue with requireSrc, it actually should probably be __require; and yes we are adding an additional symbol to the module's scope; I'm not really fond of doing that either; but sometimes simple is better. ;-)

  6. Maybe 6 is what is actually calling my global.require. My app.js ->

    
    "use strict";
    const application = require("application");
    require('nativescript-updater'); // This overrides the global.require and setups up the global.__getRequire

// Any other APP.js code

application.start({moduleName: 'main-page'});

So the global.require is overrode before ANY other requires occur after the basic framework is loaded.  This means that "main-page" will be loaded and required from the "new" directory if it exists.

To me I like the __requireOverride -- it is very simple and yet very powerful it gives me all the values I need (i.e. I get the current path, and filename).   I can do a `require` from inside it (to change the path) or use any method to read the value (FileSystem, Database, hard coded) and return that data instead, and finally if I return false; it will just use the normal require logic.      This is very simple for me to override when I need to.   

The code is simply what I put up above.  All I did was remove the two `READONLY` on the two `require` statements.   And change the module code to these four lines of code:

moduleFunctionSource.append("{function anonymous(requireSrc, module, exports, dirname, filename) {"); moduleFunctionSource.append("var require=requireSrc; if (typeof global.getRequire === 'function') { require = getRequire(requireSrc, __dirname); }"); moduleFunctionSource.append(source); moduleFunctionSource.append("\n}}");


I'm on board with whatever direction you want to go; I will continue using my above patches in my own runtimes until something is officially released.  So that people can use the NS-Updater plugin.  

If you really want the above patch, I can commit it; but that just requires extra time to clone the official repo again; reset to the github clone of the repo; then commit that part of the patch and submit it.   I don't see the point in wasting that much time unless you really do plan on using it.   

I think the code can be simplified (I haven't tried this yet) to be something like this:

moduleFunctionSource.append("{function anonymous(require, module, exports, dirname, filename) {"); moduleFunctionSource.append("if (typeof global.getRequire === 'function') { require = getRequire(require, __dirname); }");


Which eliminates the extra variable as long as the require variable is not readonly...
ivanbuhov commented 7 years ago

@NathanaelA Where is the second ReadOnly flag that you have removed (except here)?

  1. If I stop exposing global.require by commenting this line and run an example app, everything works great. This means that the global.require is never called. I still can't understand why you need to override it.
  1. What I mean is that if in your custom require you load modules without calling the original require (for example - reading file from memory or stream) you should take care for the module caching, so the next time the module is required it should not be executed again. In the way you use the require override this is not an issue because your custom module loading mechanism still calls the original require function which takes care of caching the export object. This will not be an issue with ES6 module loader hooks.

  2. The iOS Runtime supports ES6 modules (the import {} from 'module' syntax). Our main example app - Gamerraw is using this syntax. ES6 modules can live in every NativeScript iOS app out there and the require hook will not work with such modules because the modules are loaded without even calling the require function. This will not be an issue with ES6 module loader hooks.

  3. module.require works properly. It is officially supported. It works both on iOS and Android and it is documented here.

    I'm not really fond of doing that either; but sometimes simple is better. ;-) ... it is very simple and yet very powerful ...

In the context of comparing ES6 module loader hooks vs __requireOverride, I agree that __requireOverride approach is simpler for implementation. Regarding the powerfulness - I can't think of an issue that can be solved with the __requireOverride approach but not with the ES6 module loader hooks.

To me I like the __requireOverride ...

To be honest, I like it too and I also find it simple, but what I am looking for is a technical justification proving the benefits of this solution compared to alternatives (ES6 module loader hooks in this case).

Which eliminates the extra variable as long as the require variable is not readonly...

The require argument of the module wrapping function has never been readonly and we don't have plans to make it readonly.

If you really want the above patch, I can commit it; but that just requires extra time to clone the official repo again; reset to the github clone of the repo; then commit that part of the patch and submit it. I don't see the point in wasting that much time unless you really do plan on using it.

We are glad to merge every PR that comes with a technical justification and is proved to be the best solution of a given problem. As I said, my vote goes to ES6 module loader hooks but this is subject to change if I really see strong technical arguments in favor of another solution.

fealebenpae commented 7 years ago

This might be made moot by the latest decree from App Store Review: https://forums.developer.apple.com/thread/73640. Basically Apple are cracking down on apps that do hot-patching that bypasses the App Store process.

NathanaelA commented 7 years ago

@fealebenpae -- Nope, this currently doesn't concern me that much. -- This appears to be specifically targeting binary replacement (rollup & jspatch). CodePush has not seen any rejections of apps on JS frameworks. And specifically several apps have been accepted in the last couple days with CodePush embedded and Section 3.3.2 still has the exclusion for JavaScriptCore; which has NOT changed since this crackdown. ;-)

So unless they are going to get rid of the exclusion; it appears NS-Updater is still a viable product. ;-)

fealebenpae commented 7 years ago

Eh, whether it's rollup.io or codepush is just splitting hairs IMO - Apple have demonstrated they don't want apps to avoid the App Store and do self-updates. We both know that frameworks like Cordova and React Native are limited in what they can access from JavaScript, whereas there's very little NativeScript can't access in the native APIs, including private APIs. I really don't think there would be much difference from Apple's POV between the binary patching rollup.io provides and the ability to evaluate external JavaScript code in NativeScript that could completely change the behavior of an app without going through an App Store review.

Still, I'm no longer involved with NativeScript so I'm not in a position to influence the decisions the project takes with regards to allowing patching or not. Personally as a developer I would avoid a hotpatching solution out of fear of App Store rejection. I just hope the Apple crackdowns don't reach NativeScript-based applications because of the ability to hotpatch.

NathanaelA commented 7 years ago

@fealebenpae - You have a point; I was just discussing that on Slack that imho there is very little difference from what NS can do vs a binary updater. However, unless they decide to close this currently VALID update option, there is no reason not to pursue it. ;-)

Btw, if I wanted to get a "bad" app on Apple store; I would just encrypt it, using the free NS's encryption. Set the date to run the "bad" code in a month; let the benign app be approved and distributed for a month or so... Don't even need to do any updates.. :-)

NathanaelA commented 7 years ago

@ivanbuhov - I retested this.

  1. According to console.logs in my replaced global.require I got the application.start('main-page') called into it and so the global.require is what directed the main-page into the updated version directory. In addition "ui/page", "ui/action-bar", "ui/layouts/stack-layout", "ui/label", "ui/button" where all loaded by my global.require

  2. ok

  3. I'll have to look at import and what is required to override it. This might be a moot point though since I can specify in my docs that "require" is the only accepted hot patching method. Or the __requireOverride can be extended that import also calls it. I'll double check on android if it already works that way...

  4. Interesting. I'll have to see how that works on android; if I had a guess the __requireOverride is still in effect, so it doesn't cause me any issues.


I think the technical justification to do requireOverride over loaders is that it would bring feature parity to iOS doing it the same way as android, and be much simpler to implement. If you implement requireOverride then both platforms have the same support and everything can be still done the same way.

The technical justification for my patch, is it is free and you don't have to do anything at all until you have time to do ES6 loaders.

The technical justification for ES6 loaders is it will support both import and require with one set of loaders.


Second readonly is here: https://github.com/NativeScript/ios-runtime/blob/master/src/NativeScript/GlobalObject.moduleLoader.mm#L439

NathanaelA commented 7 years ago

@ivanbuhov - Just a FYI; import on android does not appear to use __requireOverride; but I honestly don't think this is that big of an issue at this point. Obviously E6 loaders would fix this; but at this point I will document that limitation in my updater, since even my patch doesn't handle the import statement.

One thing I forgot to answer in my prior post is that I honestly do see ES6 module loaders as being the best solution; however I don't want to divert you guys from more important work for a single persons request (i.e. me and my silly issue). So I'm willing to take my solution or a __requireOverride in the interim until time could be spent on the loaders and it is ready. If you want to just use my solution, until you have time to work on ES6 loaders -- I'll get you a patch... Otherwise; I don't see the point it putting up my patch and then having you reject it because it isn't what you really want to add to the code base. ;-D

I will leave it to you how you want to implement this; my current patched runtime will work fine until the official support is added. I would rather have this built in as that simplifies my distribution, but I do have a way to proceed on iOS. ;-)

ivanbuhov commented 7 years ago

@NathanaelA This line sets the sourceUrl property on the require object and I can't understand why it can't be readonly.

I admit that ES6 module loader hooks will take much more time, but if we introduce support for __requireOverride now, we have to support it forever, even after introducing ES6 module loader hooks. This is another argument against merging the PR.

However, we really appreciate your engagement and responsiveness in such discussions because discussions like this make NativeScript better.

NathanaelA commented 7 years ago

The reason why it can't be read only is because my current __getRequire (hacky) version will change the require value ->

moduleFunctionSource.append("{function anonymous(require, module, exports, __dirname, __filename) {");
moduleFunctionSource.append("if (typeof global.__getRequire === 'function') {   require = __getRequire(require, __dirname); }");

I do a require = which means that variable that comes into this function can't be marked read only. ;-) In a __requireOverride design, it can stay read only. Just my simple patch needs to be able to read/write.


You might have to keep requireOverride around for a short while; but I would guess you could mark it it as depreciated when es6 module loaders are introduced and then the following NS version remove it. I always consider WHATEVER as internal/private functions that are not really public api; they are used at my own risk. ;-)

ivanbuhov commented 7 years ago

@NathanaelA FYI: This comes from Apple Developer Program License Agreement:

3.3.2 Except as set forth in the next paragraph, an Application may not download or install executable code. Interpreted code may only be used in an Application if all scripts, code and interpreters are packaged in the Application and not downloaded. The only exceptions to the foregoing are scripts and code downloaded and run by Apple's built-in WebKit framework or JavascriptCore, provided that such scripts and code do not change the primary purpose of the Application by providing features or functionality that are inconsistent with the intended and advertised purpose of the Application as submitted to the App Store.

NathanaelA commented 7 years ago

@ivanbuhov - Yep; seen that several times! 😀 NS is using JavaScriptCore on iOS. Second, Apps using Microsoft's open source code-push are still being accepted by Apple without any issues in both Cordova and React Native apps. Third the email they sent out for the rejection was specifically calling out a couple functions that they didn't like being used.

At this moment , since they haven't disabled apps that are using CodePush; I believe I am still inside the grey area and am fine. At this point worst case is they can disable it in the future; best case is that they continue to ignore them and CodePush and NS-Updater can continue to function forever...

ivanbuhov commented 7 years ago

@NathanaelA Sure, NS is using JavaScriptCore on iOS, but not the built-in one. The iOS runtime comes with a custom build of the JSC and the exception mentioned in the License Agreement is valid only for code executed by the built-in JSC. However, I hope NS-Updater will not be affected by the newest Apple decree but still I wouldn't bet on it (personal opinion).

NathanaelA commented 7 years ago

Well, since Apple can change the terms at any time and eliminate any apps like they have done in the past I'm not that worried about that phrasing. IANAL, but I have a sister who is... It only states Apple's built-in WebKit framework or JavascriptCore. It doesn't actually state Apple's built-in JavascriptCore, just `JavascriptCore' and in this case they technically left a legal loop hole. ;-) That is why I said it was a grey area...

I assume this is also why CodePush gets away with it in ReactNative and Cordova/PhoneGap even though they are also using a custom versions of JSC also... As long as they are not attempting to block CodePush, then me being an even smaller entity probably won't get blocked...

But yes, I have already sent notices to anyone that as pre-purchased my solution and offered a full refund if they don't want to take the risk with my product. ;-) However, I tend to "believe" that Apple isn't worried about CodePush or NS-Updater as long as people use them responsibly i.e.

provided that such scripts and code do not change the primary purpose of the Application by providing features or functionality that are inconsistent with the intended and advertised purpose of the Application as submitted to the App Store

I do believe that if you see a bit of malware slip streamed into the store via CodePush or NS-Updater then there is a potential that this technique will be also banned. But at this moment, it appears to be an grey area that Apple has decided not to do anything about... ;-)

ivanbuhov commented 7 years ago

Closing the issue in favor of https://github.com/NativeScript/ios-runtime/issues/732.