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

Update ember-cli, babel and related dependencies. Fix issues. #1327

Merged
merged 1 commit into from Jun 26, 2017

Conversation

begedin
Copy link
Contributor

@begedin begedin commented Jun 12, 2017

This has been a long time coming and will close a couple of greenkeeper PRs as well as simplify our future upgrade processes.

My assumption was, the tests were failing once we update ember-cli (and with that, babel) due to a babel issue.

However, a bunch of babel issues were fixed and we still had this problem, so I did some debugging.

Turns out, our problem was doing this at a module level:

const { Object } = Ember;

This code, when declared in a module root, would somehow mess with transpiling and overwrite window.Object. At that point, a bunch of things would outright break.

I resolved this issue by simply eliminating the need for even using Object.create in those plases, by switching from someObject.get/set('prop') to get/set(someObject, 'prop'). This is our conventional approach anyway, so any code that wasn't written this way was not convention compliant to begin with.

modulePrefix: 'code-corps-ember',
environment: environment,
environment,
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 file was changed due to running ember init and manually merging. Makes it more eslint compliant.

'ember-cli-babel': {
// async, await, etc.
includePolyfill: true
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we no longer need this. Probably due to ditching phantomjs. We used to need it to make use of async/await

package.json Outdated
"ember-cli-app-version": "^3.0.0",
"ember-cli-autoprefixer": "0.7.0",
"ember-cli-babel": "^5.2.4",
"ember-cli-babel": "^6.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

6.0.0 is the one selected by ember-cli, but we should be safe to upgrade further.

@begedin begedin force-pushed the update-ember-cli-and-babel branch from aaeedeb to b278de1 Compare June 12, 2017 13:50
"ember-cli-app-version": "^3.0.0",
"ember-cli-autoprefixer": "0.7.0",
"ember-cli-babel": "^5.2.4",
"ember-cli-babel": "^6.4.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ember init sets the version to 6.0.0, but really, the problem was the transition from 5.x to 6.x, so no point not to upgrade further.

@begedin begedin force-pushed the update-ember-cli-and-babel branch 6 times, most recently from 459348c to cf1598e Compare June 20, 2017 12:51
@begedin
Copy link
Contributor Author

begedin commented Jun 20, 2017

I was too quick to consider this done.

There is an issue with ember-page-title, to which I submitted a fix - ember-cli/ember-page-title#76

There is also a resolved issue with ember-code-coverage, but a new version has not been released yet, so this is fixed by getting it directly from github. As soon as a new version is released, we should switch back.

"ember-cli-bourbon": "2.0.0-beta.1",
"ember-cli-code-coverage": "0.3.12",
"ember-cli-code-coverage": "kategengler/ember-cli-code-coverage",
Copy link
Contributor Author

@begedin begedin Jun 20, 2017

Choose a reason for hiding this comment

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

  • 0.3.12 fails, related to babel
  • The babel issue is fixed in master, but a new version has not been released yet.

package.json Outdated
@@ -75,15 +75,15 @@
"ember-modal-dialog": "2.2.0",
"ember-moment": "7.3.1",
"ember-normalize": "1.0.0",
"ember-page-title": "^3.1.2",
"ember-page-title": "^3.2.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 3.1.2 has an issue with ember-cli
  • 3.2.0 fixes it, but there's a regression to an old issue we already have. I submitted a PR to fix it.

@begedin begedin force-pushed the update-ember-cli-and-babel branch from cf1598e to d6ad68a Compare June 26, 2017 11:26
@begedin
Copy link
Contributor Author

begedin commented Jun 26, 2017

@joshsmith We should consider merging this. The tests pass, but two addons which were causing trouble have to point at github master branches in order for the tests to pass, reason being, new versions have not been released yet.

We cannot keep the old versions because they fail with new ember-cli.

Greenkeeper will create PRs as those addons get updated anyway, and this finally takes us out of the woods with the recent ember-cli update issues.

Copy link
Contributor

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

Looks good, but could use a look at my changes now @begedin.

- Fixed test and runtime issues by getting rid of explicit use of Ember.Object as a file-declared constant
- Unskip test
- Remove reference to Phantom from USAGE.md
- Improve team page
@joshsmith joshsmith merged commit d440c94 into develop Jun 26, 2017
@joshsmith joshsmith deleted the update-ember-cli-and-babel branch June 26, 2017 22:32
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

2 participants