Skip to content

[Feature] Adds mixin blueprint for module unification#16524

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
beegan:jb/mixin-blueprint
Jun 6, 2018
Merged

[Feature] Adds mixin blueprint for module unification#16524
rwjblue merged 1 commit intoemberjs:masterfrom
beegan:jb/mixin-blueprint

Conversation

@beegan
Copy link
Contributor

@beegan beegan commented Apr 16, 2018

@beegan beegan force-pushed the jb/mixin-blueprint branch 2 times, most recently from 8e4719f to e90bf46 Compare April 16, 2018 22:57
@GavinJoyce
Copy link
Member

Nice one @jackbeegan, this is looking great.

@GavinJoyce
Copy link
Member

GavinJoyce commented Apr 18, 2018

@jackbeegan and I had a chat about his idea to increase test coverage to cover the following currently untested scenarios:

  • in-repo addons
  • dummy apps (see comment below)

We both agree that it would be valuable to add this coverage in this PR, we can then use this as an example to increase coverage in a similar way for all other blueprints

@GavinJoyce
Copy link
Member

GavinJoyce commented Apr 20, 2018

@jackbeegan I added some ember g service tests for in-repo addons in classic apps: #16553. We can't yet write similar tests for MU apps as we haven't added MU support to ember g in-repo-addon yet. I'll get that done soon.

I also noticed that ember don't seem to support ember g service --dummy. I think we can safely live without dummy app generation tests for now, I'll follow up at the next st-module-unification weekly meeting and verify if it would be valuable to support this or not. Either way, we can ignore them for now

},
__testType__() {
return 'mixins';
},
Copy link
Member

@GavinJoyce GavinJoyce Apr 20, 2018

Choose a reason for hiding this comment

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

I don't think we need this. ⚡️?

});
});

describe('in in-repo-addon - module unification', function() {
Copy link
Member

Choose a reason for hiding this comment

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

I haven't yet landed ember-cli/ember-cli#7783 so you can remove this test for now, it's not currently possible to test the desired behaviour of MU in-repo-addon blueprint output. I'll be sure to follow up and add these tests later

@beegan beegan force-pushed the jb/mixin-blueprint branch from 172c2cf to 55331d9 Compare April 29, 2018 12:53
@GavinJoyce
Copy link
Member

GavinJoyce commented Apr 29, 2018

Nice one @jackbeegan, this looks great. As a last step, perhaps you could squash, rebase and remove the WIP from the title?

@beegan beegan force-pushed the jb/mixin-blueprint branch 4 times, most recently from 73b58ce to a5f051d Compare May 29, 2018 10:21
@beegan beegan changed the title [WIP] [Feature] Adds mixin blueprint for module unification [Feature] Adds mixin blueprint for module unification May 29, 2018
@GavinJoyce
Copy link
Member

@jackbeegan as a last step, can you prefix the commit message with [Feature]?

@beegan beegan force-pushed the jb/mixin-blueprint branch from a5f051d to 60ed099 Compare May 31, 2018 09:05
Copy link
Member

@GavinJoyce GavinJoyce left a comment

Choose a reason for hiding this comment

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

CI Failures seems to be flakiness

👍 nice one @jackbeegan

@GavinJoyce
Copy link
Member

@rwjblue this looks good to me and is ready for review. It seems that the test failure is just flaky CI

@beegan beegan force-pushed the jb/mixin-blueprint branch from 60ed099 to df591a6 Compare June 6, 2018 12:17
@rwjblue
Copy link
Member

rwjblue commented Jun 6, 2018

Thank you!!

@rwjblue rwjblue merged commit fe23def into emberjs:master Jun 6, 2018
@beegan beegan deleted the jb/mixin-blueprint branch June 6, 2018 12:47
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.

3 participants