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-25650] Update to babel 7 ecosystem #37

Merged
merged 5 commits into from Nov 13, 2018

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Jun 27, 2018

https://jira.appcelerator.org/browse/TIMOB-25650

  • Update to babel 7 ecosystem.
  • Add quick unit test that polyfilling works.
  • Replace wrench usage with fs-extra.
  • Bump version to 0.4.14

@sgtcoolguy
Copy link
Contributor Author

@ewanharris Note that I have not yet tried dropping this into the SDK to see if it breaks it. I simply ran the existing tests and adde one new one for polyfilling. (the tests are not particularly comprehensive here, I'd be happy if anyone had some more tests to write - maybe just a number of files to pass through jsanalyze with different options to verify the output?).

lib/jsanalyze.js Outdated
moduleDstDir = path.join(modulesDir, moduleName);
// WARNING: REMEMBER TO UPDATE THIS IF '@babel/polyfill' DEPENDENCIES CHANGE!
// Because core-js is under @babel/polyfill, it will get copied. require.resolve didn't work here for it anyways
[ '@babel/polyfill', '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.

Should we move this outside of the if (opts.transpile) { and add some logic to include core-js to always polyfill?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think the issue was raised suggesting we should just always polyfill. I happen to agree we probably should always polyfill and should enable transpile by default, but didn't try to alter that behavior here. We should open a ticket for each and consider it as a team. It may make more sense in a 8.0.0 timeline, though it shouldn't be a breaking change.

@sgtcoolguy
Copy link
Contributor Author

Sort of related: tidev/titanium-sdk#10112

Copy link
Contributor

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

Couple comments. Kitchensink currently errors out due to the core-js path in the app not being as expected.

@@ -27,21 +27,22 @@
"lib": "./lib"
},
"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be safer to pin the babel packages versions, from their previous betas breaking changes are still possible up until the RC.

lib/jsanalyze.js Outdated
moduleDstDir = path.join(modulesDir, moduleName);
// WARNING: REMEMBER TO UPDATE THIS IF '@babel/polyfill' DEPENDENCIES CHANGE!
// Because core-js is under @babel/polyfill, it will get copied. require.resolve didn't work here for it anyways
[ '@babel/polyfill', '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.

core-js residing under @babel/polyfill seems to be causing issues for me in kitchensink as the require path is core-js/modules/es7.symbol.async-iterator which isn't the location as it's under @babel/polyfill, nuking the package-lock makes puts core-js back in the top level (but causes a whole host of changes).

I think something like the below for resolving the modules might work, but would require us to bump our min Node to 8.9 for the paths option in require.resolve (so we know we're getting the exact dependency @babel/polyfill wants), this makes kitchensink happy.

const polyfillPkgJSONKLocation = require.resolve(path.join('@babel/polyfill', 'package.json'));
const polyfillPath = path.dirname(polyfillPkgJSONKLocation);
const polyfillPkgJSON = fs.readJSONSync(polyfillPkgJSONKLocation);
const polyfillDest =  path.join(modulesDir, '@babel/polyfill');
if (!fs.existsSync(polyfillDest)) {
	fs.copySync(polyfillPath, polyfillDest);
}
for (const dependency of Object.keys(polyfillPkgJSON.dependencies)) {
	const dependencyPath = path.dirname(require.resolve(dependency, { paths: [ polyfillPath ] }));
	const moduleDstDir = path.join(modulesDir, dependency);
	if (!fs.existsSync(moduleDstDir)) {
		fs.copySync(dependencyPath, moduleDstDir);
	}
}

@sgtcoolguy
Copy link
Contributor Author

@ewanharris I did not pin the babel versions but I did take your polyfill dependency copying suggestion and implemented it with recursion. I had to tweak what you had somewhat for it to work, though - and had to bump the min node to 8.9.0.

@sgtcoolguy sgtcoolguy changed the title Update to babel 7 ecosystem [TIMOB-25650] Update to babel 7 ecosystem Jul 23, 2018
@ewanharris
Copy link
Contributor

ewanharris commented Aug 7, 2018

@sgtcoolguy, I'm just catching up on things but will get round to this as soon as I can. What timeframe do you think we're looking to land this in, babel shipped what they hope was the last beta last week, and are hoping to go RC->GA fairly quick (https://github.com/babel/babel/releases/tag/v7.0.0-beta.56). Do we want to land it pre-GA or hold off til GA (maybe landing around the 8.0.0 timeframe due to the node version requirements?)

@sgtcoolguy
Copy link
Contributor Author

@ewanharris I've updated this PR. It now uses the babel 7.0.0 GA version. It also bumps the node minimum to 8.9.0 and therefore bumps the version to 1.0.0 of this package.

- Add quick unit test that polyfilling works.
- Replace wrench usage with fs-extra.
- bump min node to 8.9.0
- bump version to 1.0.0
Fix indentation in test fixtures, check for opts.filename existance in global-plugin, pass @babel/core to babel-plugin-tester to override their babel-core require
Fixes tests when emulator name contains spaces
@sgtcoolguy sgtcoolguy merged commit 3cfe434 into tidev:master Nov 13, 2018
@sgtcoolguy sgtcoolguy deleted the babel-7 branch November 13, 2018 20:52
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

3 participants