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

[ALOY-1312] Support ES6 #825

Merged
merged 19 commits into from
May 19, 2017
Merged

[ALOY-1312] Support ES6 #825

merged 19 commits into from
May 19, 2017

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Apr 21, 2017

JIRA: https://jira.appcelerator.org/browse/ALOY-1312

Description:
This PR replaces usage of uglifyjs with babel-core, babylon, and babili.

Uglify still does not handle ES6 or newer code. So to support user ES6 code, we need a ES6-aware parser (babylon).

An important note here is that there is a significant performance impact of switching. In running tests locally, the setup that "optimizes" the code takes roughly 2-3x longer with babel/babili.

This step runs through each JS file to do some pre-optimizations - like replacing OS_* with boolean values based on the target platform we're compiling for, or replacing Ti.Platform.name/osname. It then runs the code through babili to "compress" it, which primarily is about then using the injected booleans and platform values to trim out dead code.

I think this is an interesting feature that really probably should live in Titanium SDK itself. Since we're already using babili to minify and remove dead code at that level for device builds (or when you set the minify flag) I'd argue that long-term the "optimizer" plugin I converted to work with babel should Iive there and only be performed during minified or production builds.

For QE/FR:
This PR is about supporting user-written ES6 code in JS. Tests should be similar to tidev/titanium-sdk#8972 where some ES6 code should be placed in an alloy controller index.js file and make sure alloy doesn't "choke" on it when we ask it to 'compile/build'.

The easiest way to check that is to generate a standard alloy app and modify app/controllers/index.js to be:

var doClick = (e) => {
  alert($.label.text);
}

$.index.open();

Note that you're just checking the app will build, it may mess up the click behavior of the label to modify the code to be like above (and studio will complain about the syntax, see https://jira.appcelerator.org/browse/TISTUD-8764)

Also of note is that this touches the source map generation for studio debugging usage. It would be best to test debugging an alloy app with this PR generates proper source maps and behaves as expected within the studio debugger. From my testing, I generated the standard alloy app, placed breakpoints inside app/controllers/index.js lines 2 and 5 and launched on iOS. It seemed to behave as expected.

@sgtcoolguy
Copy link
Contributor Author

sgtcoolguy commented Apr 21, 2017

Relates to tidev/titanium-sdk#8972

… converted to JS source code string with concatenation
…to not minify, compact, merge consecutive var decls, minify booleans, mangle var names, and a couple other babili options - to generate more readable output code. The goal is to get decently optimized output code like Alloy did before where it detected dead code and could evaluate some expressions.
@sgtcoolguy
Copy link
Contributor Author

Awesome, so this is now working (if you ignore the "compare to known good" tests for alloy compile (since the outputs do differ). BUT, this does seem to have a noticeable performance impact right now. It'd be useful to try and track down exactly what can be done to speed up the changes, since this seems to be at least 2x slower running the tests right now.

@m1ga
Copy link
Contributor

m1ga commented Apr 25, 2017

Having the same problem with the good code check at #822 (comment) not sure why this happens

@sgtcoolguy
Copy link
Contributor Author

@m1ga The "good code check" is definitely going to differ for my patch since we're replacing the way we transform/compress the code form using uglify to using babel/babili, they generate much different code as a result...

- fiddle with the set of babili plugins used to limit to minimum to achive roughly equivalent 'compress' results as uglify usage
- try to use async to asynchronously transform sets of JS files in optimizing pass and 'deasync' to block until they're all done before moving on.
@sgtcoolguy
Copy link
Contributor Author

I pushed another commit to try and speed up "compile" by doing the JS files asynchronously and then "blocking" until they're done. This yielded a minor improvement, but we're still ultimately 2-3 times slower than before.

As a result, I think if we really want to support ES6 through alloy we should consider future/further performance reviews of the code. I see that there's a lot of synchronous calls and we could conceivably make use of async to help parallelize some of this.

Additionally, as I raised in the updated description - I'd argue that some of the functionality living now in Alloy's optimizer plugin should migrate to the SDK/CLI itself.

@sgtcoolguy sgtcoolguy requested a review from feons May 15, 2017 16:03
@brentonhouse
Copy link
Contributor

Does this PR handle the scenario where you use an import statement in your controller? Will it move these to the top of the newly generated controller file?

@feons
Copy link
Contributor

feons commented May 19, 2017

Looks good except the following error when doing import in controller.

[ERROR] Error generating AST for "/Users/feonsua/work/testapps/test/app/controllers/index.js"
[ERROR] 'import' and 'export' may appear only with 'sourceType: module' (9:0)
[ERROR] position 383

@brentonhouse
Copy link
Contributor

@feons -- That's the issue I had when using es6 before this PR. Are there any babel plugins that could move them for us?

Copy link
Contributor

@cb1kenobi cb1kenobi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR and FR'd. Looks good. APPROVED

@cb1kenobi cb1kenobi merged commit 49a629e into tidev:master May 19, 2017
@hansemannn
Copy link
Contributor

Did we solve the compile-speed issues? How do others solve it? And does it still depend on the iOS / Android version?

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

6 participants