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] <LinkTo> should link within the engine when used inside one #19223

Merged
merged 1 commit into from
Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 40 additions & 18 deletions packages/@ember/-internals/glimmer/lib/components/link-to.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
*/

import { alias, computed } from '@ember/-internals/metal';
import { getOwner } from '@ember/-internals/owner';
import RouterState from '@ember/-internals/routing/lib/system/router_state';
import { isSimpleClick } from '@ember/-internals/views';
import { assert, warn } from '@ember/debug';
import { EngineInstance, getEngineParent } from '@ember/engine';
import { flaggedInstrument } from '@ember/instrumentation';
import { inject as injectService } from '@ember/service';
import { DEBUG } from '@glimmer/env';
Expand Down Expand Up @@ -487,6 +490,13 @@ const LinkComponent = EmberComponent.extend({
init() {
this._super(...arguments);

assert(
Copy link
Contributor

Choose a reason for hiding this comment

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

great stuff!

'You attempted to use the <LinkTo> component within a routeless engine, this is not supported. ' +
'If you are using the ember-engines addon, use the <LinkToExternal> component instead. ' +
'See https://ember-engines.com/docs/links for more info.',
!this._isEngine || this._engineMountPoint !== undefined
);

// Map desired event name to invoke function
let { eventName } = this;
this.on(eventName, this, this._invoke);
Expand All @@ -497,9 +507,17 @@ const LinkComponent = EmberComponent.extend({
_currentRouterState: alias('_routing.currentState'),
_targetRouterState: alias('_routing.targetState'),

_isEngine: computed(function (this: any) {
return getEngineParent(getOwner(this) as EngineInstance) !== undefined;
}),

_engineMountPoint: computed(function (this: any) {
return (getOwner(this) as EngineInstance).mountPoint;
}),

_route: computed('route', '_currentRouterState', function computeLinkToComponentRoute(this: any) {
let { route } = this;
return route === UNDEFINED ? this._currentRoute : route;
return this._namespaceRoute(route === UNDEFINED ? this._currentRoute : route);
}),

_models: computed('model', 'models', function computeLinkToComponentModels(this: any) {
Expand Down Expand Up @@ -608,7 +626,7 @@ const LinkComponent = EmberComponent.extend({
}
),

_isActive(routerState: any) {
_isActive(routerState: RouterState): boolean {
if (this.loading) {
return false;
}
Expand All @@ -619,25 +637,17 @@ const LinkComponent = EmberComponent.extend({
return currentWhen;
}

let isCurrentWhenSpecified = Boolean(currentWhen);
let { _models: models, _routing: routing } = this;

if (isCurrentWhenSpecified) {
currentWhen = currentWhen.split(' ');
if (typeof currentWhen === 'string') {
return currentWhen
.split(' ')
.some((route) =>
routing.isActiveForRoute(models, undefined, this._namespaceRoute(route), routerState)
);
} else {
currentWhen = [this._route];
}

let { _models: models, _query: query, _routing: routing } = this;

for (let i = 0; i < currentWhen.length; i++) {
if (
routing.isActiveForRoute(models, query, currentWhen[i], routerState, isCurrentWhenSpecified)
) {
return true;
}
return routing.isActiveForRoute(models, this._query, this._route, routerState);
}

return false;
},

transitioningIn: computed(
Expand All @@ -664,6 +674,18 @@ const LinkComponent = EmberComponent.extend({
}
),

_namespaceRoute(route: string): string {
let { _engineMountPoint: mountPoint } = this;

if (mountPoint === undefined) {
return route;
} else if (route === 'application') {
return mountPoint;
} else {
return `${mountPoint}.${route}`;
}
},

/**
Event handler that invokes the link, activating the associated route.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,7 @@ moduleFor(

["@test query params don't have stickiness by default between model"](assert) {
assert.expect(1);
let tmpl = '{{#link-to "blog.category" 1337}}Category 1337{{/link-to}}';
let tmpl = '{{#link-to "category" 1337}}Category 1337{{/link-to}}';
Copy link
Member Author

Choose a reason for hiding this comment

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

So this is one way you can tell this is a breaking change. Do people really use engines without the engines addon though?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes ember-engines more magic than expected, I like the way how ember-engines are explicit on route path

cc: @dgeb @rwjblue

Copy link
Member

Choose a reason for hiding this comment

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

@chancancode I don't think we actually support engines without the ember-engines addon. All of our documentation and guidance on usage of engines has been in context of using the addon (roughly akin to how we now are allowed to assume folks use ember-source through ember-cli).

Copy link
Contributor

@villander villander Oct 29, 2020

Choose a reason for hiding this comment

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

@rwjblue and how about don't use engine.route-name anymore? I think it's a drawback for us

this.setupAppAndRoutableEngine();
this.additionalEngineRegistrations(function () {
this.register('template:category', compile(tmpl));
Expand All @@ -895,7 +895,7 @@ moduleFor(
) {
assert.expect(2);
let tmpl =
'{{#link-to "blog.author" 1337 class="author-1337"}}Author 1337{{/link-to}}{{#link-to "blog.author" 1 class="author-1"}}Author 1{{/link-to}}';
'{{#link-to "author" 1337 class="author-1337"}}Author 1337{{/link-to}}{{#link-to "author" 1 class="author-1"}}Author 1{{/link-to}}';
this.setupAppAndRoutableEngine();
this.additionalEngineRegistrations(function () {
this.register('template:author', compile(tmpl));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
import { moduleFor, ApplicationTestCase, runLoopSettled, runTask } from 'internal-test-helpers';
import {
ApplicationTestCase,
ModuleBasedTestResolver,
moduleFor,
runLoopSettled,
runTask,
} from 'internal-test-helpers';
import Controller, { inject as injectController } from '@ember/controller';
import { A as emberA, RSVP } from '@ember/-internals/runtime';
import { alias } from '@ember/-internals/metal';
import { subscribe, reset } from '@ember/instrumentation';
import { Route, NoneLocation } from '@ember/-internals/routing';
import { EMBER_IMPROVED_INSTRUMENTATION } from '@ember/canary-features';
import Engine from '@ember/engine';
import { DEBUG } from '@glimmer/env';
import { compile } from '../../../utils/helpers';

// IE includes the host name
function normalizeUrl(url) {
Expand Down Expand Up @@ -348,6 +356,209 @@ moduleFor(
);
});
}

async ['@test Using <LinkTo> inside a non-routable engine errors'](assert) {
this.add(
'engine:not-routable',
class NotRoutableEngine extends Engine {
Resolver = ModuleBasedTestResolver;

init() {
super.init(...arguments);
this.register(
'template:application',
compile(`<LinkTo @route='about'>About</LinkTo>`, {
moduleName: 'non-routable/templates/application.hbs',
})
);
}
}
);

this.addTemplate('index', `{{mount 'not-routable'}}`);

await assert.rejectsAssertion(
this.visit('/'),
'You attempted to use the <LinkTo> component within a routeless engine, this is not supported. ' +
'If you are using the ember-engines addon, use the <LinkToExternal> component instead. ' +
'See https://ember-engines.com/docs/links for more info.'
);
}

async ['@test Using <LinkTo> inside a routable engine link within the engine'](assert) {
this.add(
'engine:routable',
class RoutableEngine extends Engine {
Resolver = ModuleBasedTestResolver;

init() {
super.init(...arguments);
this.register(
'template:application',
compile(
`
<h2 id='engine-layout'>Routable Engine</h2>
{{outlet}}
<LinkTo @route='application' id='engine-application-link'>Engine Appliction</LinkTo>
`,
{
moduleName: 'routable/templates/application.hbs',
}
)
);
this.register(
'template:index',
compile(
`
<h3 class='engine-home'>Engine Home</h3>
<LinkTo @route='about' id='engine-about-link'>Engine About</LinkTo>
<LinkTo @route='index' id='engine-self-link'>Engine Self</LinkTo>
`,
{
moduleName: 'routable/templates/index.hbs',
}
)
);
this.register(
'template:about',
compile(
`
<h3 class='engine-about'>Engine About</h3>
<LinkTo @route='index' id='engine-home-link'>Engine Home</LinkTo>
<LinkTo @route='about' id='engine-self-link'>Engine Self</LinkTo>
`,
{
moduleName: 'routable/templates/about.hbs',
}
)
);
}
}
);

this.router.map(function () {
this.mount('routable');
});

this.add('route-map:routable', function () {
this.route('about');
});

this.addTemplate(
'application',
`
<h1 id='application-layout'>Application</h1>
{{outlet}}
<LinkTo @route='application' id='application-link'>Appliction</LinkTo>
<LinkTo @route='routable' id='engine-link'>Engine</LinkTo>
`
);

await this.visit('/');

assert.equal(this.$('#application-layout').length, 1, 'The application layout was rendered');
assert.strictEqual(this.$('#engine-layout').length, 0, 'The engine layout was not rendered');
assert.equal(this.$('#application-link.active').length, 1, 'The application link is active');
assert.equal(this.$('#engine-link:not(.active)').length, 1, 'The engine link is not active');

assert.equal(this.$('h3.home').length, 1, 'The application index page is rendered');
assert.equal(this.$('#self-link.active').length, 1, 'The application index link is active');
assert.equal(
this.$('#about-link:not(.active)').length,
1,
'The application about link is not active'
);

await this.click('#about-link');

assert.equal(this.$('#application-layout').length, 1, 'The application layout was rendered');
assert.strictEqual(this.$('#engine-layout').length, 0, 'The engine layout was not rendered');
assert.equal(this.$('#application-link.active').length, 1, 'The application link is active');
assert.equal(this.$('#engine-link:not(.active)').length, 1, 'The engine link is not active');

assert.equal(this.$('h3.about').length, 1, 'The application about page is rendered');
assert.equal(this.$('#self-link.active').length, 1, 'The application about link is active');
assert.equal(
this.$('#home-link:not(.active)').length,
1,
'The application home link is not active'
);

await this.click('#engine-link');

assert.equal(this.$('#application-layout').length, 1, 'The application layout was rendered');
assert.equal(this.$('#engine-layout').length, 1, 'The engine layout was rendered');
assert.equal(this.$('#application-link.active').length, 1, 'The application link is active');
assert.equal(this.$('#engine-link.active').length, 1, 'The engine link is active');
assert.equal(
this.$('#engine-application-link.active').length,
1,
'The engine application link is active'
);

assert.equal(this.$('h3.engine-home').length, 1, 'The engine index page is rendered');
assert.equal(this.$('#engine-self-link.active').length, 1, 'The engine index link is active');
assert.equal(
this.$('#engine-about-link:not(.active)').length,
1,
'The engine about link is not active'
);

await this.click('#engine-about-link');

assert.equal(this.$('#application-layout').length, 1, 'The application layout was rendered');
assert.equal(this.$('#engine-layout').length, 1, 'The engine layout was rendered');
assert.equal(this.$('#application-link.active').length, 1, 'The application link is active');
assert.equal(this.$('#engine-link.active').length, 1, 'The engine link is active');
assert.equal(
this.$('#engine-application-link.active').length,
1,
'The engine application link is active'
);

assert.equal(this.$('h3.engine-about').length, 1, 'The engine about page is rendered');
assert.equal(this.$('#engine-self-link.active').length, 1, 'The engine about link is active');
assert.equal(
this.$('#engine-home-link:not(.active)').length,
1,
'The engine home link is not active'
);

await this.click('#engine-application-link');

assert.equal(this.$('#application-layout').length, 1, 'The application layout was rendered');
assert.equal(this.$('#engine-layout').length, 1, 'The engine layout was rendered');
assert.equal(this.$('#application-link.active').length, 1, 'The application link is active');
assert.equal(this.$('#engine-link.active').length, 1, 'The engine link is active');
assert.equal(
this.$('#engine-application-link.active').length,
1,
'The engine application link is active'
);

assert.equal(this.$('h3.engine-home').length, 1, 'The engine index page is rendered');
assert.equal(this.$('#engine-self-link.active').length, 1, 'The engine index link is active');
assert.equal(
this.$('#engine-about-link:not(.active)').length,
1,
'The engine about link is not active'
);

await this.click('#application-link');

assert.equal(this.$('#application-layout').length, 1, 'The application layout was rendered');
assert.strictEqual(this.$('#engine-layout').length, 0, 'The engine layout was not rendered');
assert.equal(this.$('#application-link.active').length, 1, 'The application link is active');
assert.equal(this.$('#engine-link:not(.active)').length, 1, 'The engine link is not active');

assert.equal(this.$('h3.home').length, 1, 'The application index page is rendered');
assert.equal(this.$('#self-link.active').length, 1, 'The application index link is active');
assert.equal(
this.$('#about-link:not(.active)').length,
1,
'The application about link is not active'
);
}
}
);

Expand Down
Loading