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-24610] Support transpiling user JS code to ES5, or minimum target OS/JS engine support #9512

Merged
merged 13 commits into from Jan 29, 2018

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Oct 9, 2017

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

Description: Support transpiling user JS code to ES5, or minimum target OS/JS engine support

  • Use babel-core and babel-preset-env to transpile user code during builds
  • For iOS, use the 'iOS' babel-preset target matching our iOS min sdk version (8 currently) if using jscore, otherwise set no target (which should mean transpile everything down to es5).
  • For Android, use the chrome target matching the v8 version we're on. (i.e. v8 6.2.x -> Chrome 62)
  • I tried to retain as much of the "incremental build" logic as I could. Basically, we minify/transpile if necessary, then compare the output versus the original file. If no changes, we don't write the new contents - we symlink/copy as possible. On iOS, we even check if the new contents differ from the existing contents at the destination to determine if we need to write the file out or not.

@hansemannn
Copy link
Collaborator

hansemannn commented Nov 20, 2017

Tests

iOS

  • Build for simulator
  • Build for device
  • Build for ad-hoc
  • Build for app-store
  • Incremental build

Android

  • Build for simulator
  • Build for device
  • Build for play-store
  • Incremental build

Windows

(@ewanharris / @infosia, can you help here?)

  • Build for simulator
  • Build for device
  • Build for store
  • Incremental build

Alloy

  • iOS FAILED
  • Android
  • Windows

Test-Case

class Application {
        
    constructor(cb) { 
        this.window = Ti.UI.createWindow({
            backgroundColor: '#fff'
        })

        var btn = Ti.UI.createButton({
            title: 'Trigger'
        });

        btn.addEventListener('click', () => {
            Ti.API.info('Hello world!');
        });

        this.window.add(btn);
    }
    
    open(cb) {
        this.window.open();
    }
}

var app = new Application();
app.open();

Doing more advanced tests now.

@hansemannn
Copy link
Collaborator

@sgtcoolguy Did you test against live-view so far?

@sgtcoolguy
Copy link
Contributor Author

sgtcoolguy commented Nov 21, 2017

@hansemannn I did not. I did simple smoke tests to verify the test app and basic app worked on iOS/Android and that we used the correct target presets based on the platform/engine.

I honestly have no idea how liveview works with something like minification/transpilation (ala Alloy).

@ewanharris
Copy link
Collaborator

@hansemannn For this to be tested against Windows it will need to be added to the build there.

@hansemannn
Copy link
Collaborator

Tried using the PR on studio, but using the latest studio update, it now sees the ES6 syntax as an error and the iOS build options are not available anymore..

@sgtcoolguy
Copy link
Contributor Author

Studio is still not ES6-aware, which is an entirely separate issue. I have a long-standing feature branch to replace the JS parser in Studio with Oracle's ES6-compliant parser, but I don't know when Kondal and company will be able to pick that up and get it done. (or me, if that's the next priority for me).

@build build added the ios label Nov 27, 2017
@build
Copy link
Contributor

build commented Nov 27, 2017

Warnings
⚠️

🔒 Changes were made to package.json, but not to package-lock.json - Perhaps you need to run npm install?

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@sgtcoolguy
Copy link
Contributor Author

@hansemannn @janvennemann ping. Are we good for QE?

@hansemannn
Copy link
Collaborator

hansemannn commented Nov 30, 2017

We should sync it with the Alloy- and Studio-changes, otherwise devs will be confused about our actual support, as most of them use Alloy these days. Still wasn't able to verify Alloy, so maybe @feons could help here. This PR is approved though!

@janvennemann
Copy link
Contributor

Agree with Hans, this PR looks good but i'd say we need to fix the Alloy issues first so QE can test everything at once.

Also tested with Alloy on Android and that errors out with Uncaught TypeError: Cannot read property '_' of undefined. That's caused by the replacement of this with undefined and can be solved by updating underscore.

@sgtcoolguy
Copy link
Contributor Author

I understand we want all the different parts to support es6+ at once, but I strongly disagree with holding this PR (or a Studio or Alloy one) for the others if they're not explicitly required by the others.

We have separate tickets for adding support for ES6 to the various tools and products. If we hold off until hey all support ES6 we won't make any progress for a long time.

There's still value in getting this working for classic apps built by the CLI or the new atom plugins.
If you're saying that this PR explicitly breaks alloy apps, then that is a separate issue and I can try and address that by effectively disabling transpilation (or parts of it) for alloy apps until we fix Alloy.

@sgtcoolguy
Copy link
Contributor Author

It looks like tidev/alloy#852 and tidev/alloy#842 are related then?

@hansemannn
Copy link
Collaborator

@sgtcoolguy You are right, it's probably better to unblock general ES6 (do we support ES7 and async / await with this as well?) here and do Alloy separately. I think it's also a good reason for an Alloy v2 to support ES6, so people will know that there are breaking changes like the above.

@janvennemann
Copy link
Contributor

janvennemann commented Dec 6, 2017

@sgtcoolguy, sorry if it wasn't clear, but yeah i meant that this breaks Alloy and we should at least solve that. The PRs you mentioned should do exactly that. My intention was not hold everything off until ES6 support is available for all but at least keep things working :)

@hansemannn
Copy link
Collaborator

Just tested it with Alloy and if we don't use ES6, the build seems to be fine. So we could merge this one and do Alloy v2 with the updated underdash / lowdash next?

@build build added the ios label Dec 6, 2017
@hansemannn
Copy link
Collaborator

I've updated the lodash PR to resolve the merge conflicts, let's do this!

…ode down to target JS engine. Start with Android, hard-code to the equivalent chrome version based on teh v8 version we currently use. Tweak logic for JS file to try and symlink if we can and there are no changes from transpiling and/or minification; otherwise just write new contents to dest.
…older, so that we can access it for transpile target info
…hnically breaks an undocumented and unofficial way to hang things off global via this binding in required scripts
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