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.

Sign up for GitHub

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

Jump to bottom

Remove LinkTo's tagName #19662

Closed
wants to merge 2 commits into from
Closed

Remove LinkTo's tagName #19662

wants to merge 2 commits into from

Conversation

nlfurniss
Copy link
Contributor

Part of #19617

Deprecation Guide

Tests pass locally

@mixonic mixonic mentioned this pull request Jul 19, 2021
Closed
58 tasks
mixonic
mixonic reviewed Jul 20, 2021
View reviewed changes
packages/@ember/-internals/glimmer/lib/components/link-to.ts
}
);

this.modernized = false;
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It is asserted that passing <LinkTo @tagName="div"> still renders an <a> after this removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I added an assert to the validateArguments method and some tests that assert for this error, but not sure if that's the right place to throw versus where @href throws

@rwjblue
Copy link
Member

rwjblue commented Jul 21, 2021

Needs a rebase (one of the other PR's must have caused a conflict).

mixonic
mixonic reviewed Jul 22, 2021
View reviewed changes
packages/@ember/-internals/glimmer/lib/components/link-to.ts
Comment on lines +83 to +86
assert(
'Passing the `@tagName` argument to <LinkTo> is not supported.',
!('tagName' in this.args.named)
);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Two concerns:

With this implemented as an assert, what does that mean for production? Would a @tagName be applied in production, even if it the assert stopped you in development? Seems bad if so. Maybe it should be a throw.

But, further, IMO we don't need a check at all. I don't think we're planning to add asserts for every old component argument which was removed in 4.0. They were removed, which means passing one of those arguments (like @tagName) could just mean it is ignored. I can ask around on this second point, but the first concern about production behavior not applying the tag name needs to be confirmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, makes sense re: assert. As to the second point, classic components will keep tagName, so it seems like something special will need to be done for linkTo.

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 22, 2021

FYI @nlfurniss I rebased and force pushed this branch.

mixonic
mixonic requested changes Jul 22, 2021
View reviewed changes
Copy link
Sponsor Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

Per #19669 (comment) we should hold off on this until the built-in components are extracted. @chancancode has kicked off that work I believe.

This was referenced Jul 22, 2021
Merged
Merged
Merged
@bertdeblock bertdeblock mentioned this pull request Sep 23, 2021
Closed
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 4, 2021
@mixonic
Drop export of built-ins, remove legacy components
d27c578 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
@mixonic mixonic mentioned this pull request Nov 4, 2021
Merged
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 4, 2021
@mixonic
Drop export of built-ins, remove legacy components
f2f2931 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 4, 2021
@mixonic
Drop export of built-ins, remove legacy components
5b38e47 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 4, 2021
@mixonic
Drop export of built-ins, remove legacy components
e348f9b 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 8, 2021
@mixonic
Drop export of built-ins, remove legacy components
b6923c0 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 8, 2021
@mixonic
Drop export of built-ins, remove legacy components
f038bee 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 9, 2021
@mixonic
Drop export of built-ins, remove legacy components
760c91d 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
mixonic added a commit to mixonic/ember.js that referenced this pull request Nov 9, 2021
@mixonic
Drop export of built-ins, remove legacy components
5d24ae7 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review emberjs#19669 and ensure all
  `disabledWhen` is removed.
* Review emberjs#19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts
kategengler pushed a commit that referenced this pull request Nov 10, 2021
@mixonic @kategengler
Drop export of built-ins, remove legacy components
3e6c348 
* Remove the export of `@ember/component/checkbox`
* Remove the export of `@ember/component/text-field`
* Remove the export of `@ember/component/text-area`
* Remove the export of `@ember/component/link-component`
* Remove the assignment of `Ember.Checkbox`
* Remove the assignment of `Ember.TextField`
* Remove the assignment of `Ember.TextArea`
* Remove the assignment of `Ember.LinkComponent`
* Remove the assignment of `Ember.TextSupport`
* Remove the assignment of `Ember.TargetActionSupport`
* Hard-code `EMBER_MODERNIZED_BUILT_IN_COMPONENTS` to `true` path in
  most places. The remaining are a few spots where there are
  deprecations targeting *5.0*.
* Hard-code the built-in components (`<Textarea>`, `<Input>`,
  `<LinkTo>`) for the modernized path
* Remove the `query-params` helper (it is only needed by the legacy
  `LinkComponent` and was deprecated).
* Review #19669 and ensure all
  `disabledWhen` is removed.
* Review #19662 and ensure all
  `@tagName` support is removed
* Review all `ember.built-in-components.*` deprecations and ensure their
  functionality is removed.
* Remove code supporting `ember-glimmer.link-to.positional-arguments`,
  basically
  https://github.com/emberjs/ember.js/blob/master/packages/ember-template-compiler/lib/plugins/transform-link-to.ts

(cherry picked from commit 5d24ae7)
@mixonic
Copy link
Sponsor Member

mixonic commented Nov 12, 2021

Supplanted by #19806. Thank you @nlfurniss !

@mixonic mixonic closed this Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mixonic mixonic mixonic requested changes

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
4.0 Cleanup Cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nlfurniss @rwjblue @mixonic @kategengler