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-26117] Use polyfill #32

Closed
wants to merge 5 commits into from

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Jun 12, 2018

  • Include babel-polyfil with Titanium builds
  • Use babel-preset-minify
  • Support async await

JIRA Ticket

lib/jsanalyze.js Outdated
@@ -171,7 +173,34 @@ exports.analyzeJs = function analyzeJs(contents, opts) {
// transpile
if (opts.transpile) {
options.plugins.push(require.resolve('./global-this'));
options.presets.push([ env, { targets: opts.targets } ]);
options.presets.push([ env, { targets: opts.targets, useBuiltIns: 'entry' } ]);
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the docs useBuiltIns doesn't take the entry/usage args in 6.X, only in 7.X. It's a boolean in 6.X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

lib/jsanalyze.js Outdated
// install polyfill
let child = null;
if (opts.resourcesDir) {
if (process.platform === 'win32') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing as we have the dependency added to the package.json, could we potentially copy it over to the app, it might have fewer potential problems that spawning npm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I was going to do initially, but I would need to manually copy over each dependency which could change. Where npm does this for us and keeps polyfill up-to-date.

Maybe it would be best to setup a symbolic link to polyfill? (on OSX anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

First, I think it's important we install the exact version of polypill that is included/picked in the node-titanium-sdk install/dependency tree. As is, this will install the latest, which could be breaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Second, I think this is a case where you'd want to use child_process.fork() since we really want to fork off another node process that is the same node install as currently being used. Obviously, that's an async API, so there's an issue there...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we discussed on Teams chat - but it may be better to just copy the babel-polyfill dependency tree over instead of doing an npm install. This could possibly achieve that: https://www.npmjs.com/package/copy-node-modules

@garymathews
Copy link
Contributor Author

garymathews commented Jun 12, 2018

This will also allow support of async await

TEST CASE
const win = Ti.UI.createWindow({
        backgroundColor: 'gray'
    }),
    view = Ti.UI.createView({
        backgroundColor: 'red',
        width: 200,
        height: 200
    }),
    matrix = Ti.UI.create2DMatrix({
        rotate: 90
    }),
    animation = Ti.UI.createAnimation({
        transform: matrix,
        duration: 3000
    }),
    animate = (view, animation) => {
        return new Promise(resolve => view.animate(animation, resolve));
    };

win.addEventListener('postlayout', async () => {
    await animate(view, animation);
    view.backgroundColor = 'orange';
    alert('DONE ANIMATION!');
});

win.add(view);
win.open();

lib/jsanalyze.js Outdated
}

// copy over polyfill and its dependencies
[ 'babel-polyfill', 'core-js', 'regenerator-runtime' ].forEach((moduleName) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

So obviously this is much simpler than using npm install or that npm package I pointed at before about copying dependencies over. And it looks like for now this is comprehensive - but it does scare me for future maintenance that we're essentially hard-coding the known dependencies in an array here and not doing a more general solution that would adapt if the babel-polyfill dependencies changed in future releases.

I think at the very least we may want to leave a nice comment/scary notice here that that's exactly what we're doing and to be aware if updating babel-polyfill to look at/modify this logic.

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

CR approved. LGTM

@sgtcoolguy
Copy link
Contributor

Merged manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants