Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-25790] iOS: Liveview broken when transpiling code

GitHub Issuen/a
TypeBug
PriorityHigh
StatusClosed
ResolutionFixed
Resolution Date2018-08-15T11:09:58.000+0000
Affected Version/sRelease 7.1.0, Release 7.2.0
Fix Version/sn/a
ComponentsiOS, LiveView
Labelsn/a
ReporterEwan Harris
AssigneeEwan Harris
Created2018-02-20T15:33:35.000+0000
Updated2018-08-15T11:10:04.000+0000

Description

Description

Using liveview with the <transpile> flag in the tiapp set to true (or not in the tiapp as it defaults to true) currently throws the below error [link to source](https://github.com/appcelerator/liveview/blob/713d920a19f9a6b9a444f3de53d92118ad1439f3/build/liveview.js#L427) - This occurs because the liveview code is attempting to overwrite the global.L variable, globals are defined as readOnly [see here](https://github.com/appcelerator/titanium_mobile/blob/5ce893c20d97b8d62b100d331fa40c88744df2f2/iphone/Classes/KrollContext.m#L1101) and as all code in now run in strict mode this is now an error (credit to TIMOB-18465 for helping me figure this one out) There's two ways we can fix this 1. Remove kTiPropertyAttributeReadOnly on global properties 2. Stop making all JS core run in strict mode by default by adding modules: false to the object passed to preset-env, which will have a knock on effect to ES6 module support My vote goes for 1, but again this highlights the fact the transpilation step *_is going to cause breaking changes for people_*
[ERROR] Script Error {
[ERROR]     column = 12;
[ERROR]     line = 427;
[ERROR]     message = "Attempted to assign to readonly property.";
[ERROR]     sourceURL = "file:///Users/eharris/Library/Developer/CoreSimulator/Devices/19A348C6-7F86-4096-AEDE-90BB291BC971/data/Containers/Bundle/Application/D00FE479-9C4B-4ECD-84B6-1C1790C822FE/ticreateapp.app/app.js";
[ERROR] }

Steps to reproduce

Enable transpilation by adding the <transpile>true</transpile> flag to your tiapp.xml

Build an app using liveview to iOS

Actual

Error above is thrown

Expected

No error thrown, liveview should work as normal

Comments

  1. Ewan Harris 2018-02-20

    cc [~cwilliams] [~hknoechel] If we take option 1 I think we should also fix this on all properties? Pier also filed TIMOB-18475 for String. properties, would probably be sensible to check what's globals are writable on all platforms and align for parity I should also note that I've done both options and they both work
  2. Christopher Williams 2018-02-20

    So, I think there's still a way to "fix" liveview to hack the L function in a way that doesn't try to assign to the global "L" function: It appears to hijack require itself, and in that it can simply pass a reference to "L" that invokes it's own override (right here: https://github.com/appcelerator/liveview/blob/713d920a19f9a6b9a444f3de53d92118ad1439f3/build/liveview.js#L733). That does mean user code could insist on the original L function versus a possibly patched one:
       global.L('my string'); // will always invoke the original method
       L('my string'); may invoke the overriden live view version
       
    I mean this is effectively how it is passing along it's hacked require function, so I don't see why we couldn't do the same for the L override. I guess this raises a couple larger points: - Yes, transpilation could still break user code if they're doing very funky things like this that relied on non-strict behavior (i.e. actually overriding read-only globals!). In non-strict code though, wouldn't this hijacking of L actually silently fail? If the global L function is read-only non-strict code typically would silently ignore bad behavior and strict throws errors. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode - Do we want to open up some of our API to be non read-only? Clearly liveview has a use for overriding some builtin API members. But this is beginning to play with fire if we don't want some/all of our API to be read-only.
  3. Ewan Harris 2018-02-20

    I think one of the problems here is that we're not consistent across platforms, this code works on both Windows and Android (in strict mode) who allow global.L to be overwritten. iOS is the only platform which doesn't and the docs don't define it as read-only either. Looking at other globals in other environments node doesn't seem to complain when overriding in strict mode (from my fairly limited testing). I feel that, while this is fixable in liveview, given that (to my knowledge) we wouldn't be able ship the fix alongside 7.1.0 and that iOS is in the minority of how it behaves, we should be removing the kTiPropertyAttributeReadOnly on the global properties.
  4. Ewan Harris 2018-02-27

    Thought about it and agree with you [~cwilliams], I think parts of the global namespace should be read-only while others shouldn't (Ti/Titanium should be read-only for sure). Seeing as that's a problem for another day I've put up a fix for liveview, https://github.com/appcelerator/liveview/pull/109, tested across iOS and Android just need to test Windows
  5. Ewan Harris 2018-02-28

    Progressing to in review, have tested across Android, iOS and Windows and it works
  6. Ewan Harris 2018-08-15

    This fix has now shipped in Studio 5.1.0 which was released yesterday. Please read the release blogpost for instructions on how to update, https://www.appcelerator.com/blog/2018/08/ga-release-appcelerator-studio-5-1-0/ As it's shipped I'm now closing this ticket

JSON Source