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

[BUGFIX] {{link-to}} before application boot #11639

Merged
merged 2 commits into from
Jul 3, 2015

Conversation

nathanhammond
Copy link
Member

Previously, {{link-to}} would work even if the application had not been
booted completely. This behavior is relied on by test harnesses like
ember-qunit, since an “integration test” of a component may include a
template that uses {{link-to}}. Many people have discovered that they
would like to test a component that has a link without booting the
entire app.

You wanted a banana but what you got was a gorilla holding the banana
and the entire jungle. –Joe Armstrong

During the Glimmer work, we refactored the interface between the
LinkComponent and the router into a service. Unfortunately, this
caused a regression where rendering the link before the router was
completely initialized would raise an exception.

This commit adds a very stupid guard to ensure that we have enough
router state to determine if a link is active or not. This whole path
could be significantly cleaned up, and in particular, we should move
towards using a stubbed routing service in tests that encapsulates all
of these concerns.

Conflicts:
packages/ember/tests/helpers/link_to_test.js

@rwjblue
Copy link
Member

rwjblue commented Jul 2, 2015

This was initially targeted at the beta channel because @tomdale was not super confident with this change.

I'm not opposed to pulling it into stable, but he should definitely chime in...

@nathanhammond
Copy link
Member Author

The consequence of leaving this as a 2.0-only solution is that we can't have any tests of components with {{link-to}} inside of 1.13. I'm actually surprised that there hasn't been a more-general revolt about the inability to test components with links in them in 1.13... and if that means very few people are moving to 1.13 yet, or they aren't testing...

@nathanhammond nathanhammond force-pushed the reapply-11522 branch 2 times, most recently from c60583f to 287a0a0 Compare July 2, 2015 20:05
Previously, {{link-to}} would work even if the application had not been
booted completely. This behavior is relied on by test harnesses like
ember-qunit, since an “integration test” of a component may include a
template that uses {{link-to}}. Many people have discovered that they
would like to test a component that has a link without booting the
entire app.

> You wanted a banana but what you got was a gorilla holding the banana
> and the entire jungle. –Joe Armstrong

During the Glimmer work, we refactored the interface between the
LinkComponent and the router into a service. Unfortunately, this
caused a regression where rendering the link before the router was
completely initialized would raise an exception.

This commit adds a very stupid guard to ensure that we have enough
router state to determine if a link is active or not. This whole path
could be significantly cleaned up, and in particular, we should move
towards using a stubbed routing service in tests that encapsulates all
of these concerns.

Conflicts:
	packages/ember/tests/helpers/link_to_test.js
@nathanhammond
Copy link
Member Author

Tests are coming separately based on my self-destructing reproduction so that I can more easily land them in both master and stable.

@rwjblue
Copy link
Member

rwjblue commented Jul 2, 2015

👍

@nathanhammond
Copy link
Member Author

Assuming tests pass this is good to land now, it includes a test for the last scenario. I don't believe that an acceptance test for this is required at this point.

@rwjblue
Copy link
Member

rwjblue commented Jul 3, 2015

Restarted sauce labs job.

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