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

Implement resolveEngine and resolveRouteMap #151

Merged
merged 1 commit into from
Sep 3, 2016

Conversation

trentmwillis
Copy link
Member

Addressing ember-engines/ember-engines#10.

One change to note: The route-map lookup now expects an isRouteMap property instead of trying to share a symbol between this addon and ember-engines.

cc @dgeb @rwjblue @nathanhammond

@rwjblue
Copy link
Member

rwjblue commented Sep 1, 2016

Using the stamp seems totally fine to me. Can you add a test for that assertion though?

@trentmwillis
Copy link
Member Author

@rwjblue added a test

@nathanhammond
Copy link
Contributor

My only concern is that this sneaks by the feature flag and exists as "dormant" code in apps the moment we release a new version. I'm tempted to delay landing + release until the feature flag is removed.

@nathanhammond
Copy link
Contributor

(Note: code is perfectly reasonable.)

@trentmwillis
Copy link
Member Author

@nathanhammond I'm not sure I agree. Since Ember Beta is currently running with the feature flag enabled by default, I think now would be a good time to start testing this out as well. We can release a beta version of this addon if needed, but I think we should try to have this working by the time Engines are released in 2.8 (so that we can encourage this pattern over the custom resolver provided by the ember-engines addon).

@nathanhammond
Copy link
Contributor

The fact that it'd be a beta of a point release feels ridiculous, but I'd rather match the current Ember beta progress.

@rwjblue is the only one with publish privs, so it'll be his call. 😛

@rwjblue
Copy link
Member

rwjblue commented Sep 2, 2016

emberjs/ember.js#14199

@trentmwillis
Copy link
Member Author

So @rwjblue does that mean this can land 😉

@rwjblue rwjblue merged commit fb81713 into ember-cli:master Sep 3, 2016
@rwjblue
Copy link
Member

rwjblue commented Sep 3, 2016

Released as v2.1.0

@nathanhammond
Copy link
Contributor

I now relinquish my stick in the mud position. 😄

@ilucin
Copy link

ilucin commented Sep 6, 2016

@rwjblue You should probably point out somewhere that this release doesn't work with engines < 0.3.0

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.

4 participants