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

[FEATURE modernized-built-in-components] Tracking Issue #19270

Closed
10 of 16 tasks
chancancode opened this issue Nov 14, 2020 · 2 comments
Closed
10 of 16 tasks

[FEATURE modernized-built-in-components] Tracking Issue #19270

chancancode opened this issue Nov 14, 2020 · 2 comments

Comments

@chancancode
Copy link
Member

chancancode commented Nov 14, 2020

The remaining tasks, in no particular order (most of these can be parallelized)

Some guidance on each of these...

Merge #19218 (🔒 @rwjblue)

Should be good to go, as far as I can tell.

Enable the deprecations introduced in #19218

The PR introduced a few new deprecations, but they are disabled even when the feature flag is enabled, by hard coding the deprecation condition to true. In each case though, I left the actual condition in a JS comment.

The reason that these deprecations were not enabled (behind the feature flag) as part of the original PR was that those features are used in the internal test suite, so enabling them would require a modest amount of work fixing up the existing tests.

The deprecations are:

  • Passing the @id argument to is deprecated. Instead, please pass the attribute directly, i.e. <Input id={{...}} /> instead of <Input @id={{...}} /> or {{input id=...}}. (id is just an example here, this applies to other attributes as well.)
  • Passing the @change argument to is deprecated. This would have overwritten the internal change method on the component and prevented it from functioning properly. Instead, please use the {{on}} modifier, i.e. <Input {{on "change" ...}} /> instead of <Input @change={{...}} /> or {{input change=...}}. (change is just an example here, this applies to other event methods as well.)
  • Passing the @click argument to is deprecated. Instead, please use the {{on}} modifier, i.e. <Input {{on "click" ...}} /> instead of <Input @click={{...}} /> or {{input click=...}}. (click is just an example here, this applies to other event as well.)
  • Passing the @key-down argument to is deprecated. Instead, please use the {{on}} modifier, i.e. <Input {{on "keydown" ...}} /> instead of <Input @key-down={{...}} /> or {{input key-down=...}}. (key-down is just an example here, this applies to other virtual event as well.)

In each case, we will need...

  • At least one test verifying that the deprecation is being issued correctly when the condition is met, using the expectDeprecation test helper and stuff. Naturally, this test will only pass with the feature enabled, so you can guard it with @feature(EMBER_MODERNIZED_BUILT_IN_COMPONENTS). There are some examples in [FEATURE modernized-built-in-components] First-pass implementation #19218.
  • Make sure the existing test suite pass without any unexpected deprecations, both with and without turning on the feature flag. If the test in question is there to verify the deprecated feature, you can probably sprinkle in some expectDeprecation and add a new test for the replacement feature if there isn't one already. If the test is not specifically about the deprecated feature and is just incidentally using that feature, you can refactor the test to use the replacement feature instead.

In general...

  • There is a checkbox in the QUnit UI for running the tests with or without the optional features enabled.
  • You can guard the test with @feature(EMBER_MODERNIZED_BUILT_IN_COMPONENTS) or @feature(!EMBER_MODERNIZED_BUILT_IN_COMPONENTS) to allow them to run only when the feature is enabled/disabled, respectively.
  • It is okay to "fork" an existing tests by duplicating it, making one version for when the feature is enabled and another when the feature is disabled. When the feature flag is removed, we will just remove the old test and change the new one back to @test.
  • Alternatively, you can import the EMBER_MODERNIZED_BUILT_IN_COMPONENTS from @ember/canary-features and guard a specific part of the test method in a fine-grained manner, using a regular if (EMBER_MODERNIZED_BUILT_IN_COMPONENTS) { ... } else { ... }.

Address #19218 (comment)

This is probably semi-blocked until contextual modifiers have landed. (:lock: @pzuraq)

In order to address the concern, we'd probably have to move the registration of event handlers into JavaScript which requires writing some kind of modifier. #19269 adds an API for this, but until we have contextual modifiers we would have to make the modifier available in the global namespace using some "private" name like {{-deprecated-input-event-handlers ...}} or something.

We can do that, but given that contextual modifiers are pretty close, we probably should just wait. If that didn't turn out to be the case we can 🔓 and revisit this.

Identify, implement and deprecate other classic component features

The implementation in #19218 accounted for the classic component features that are currently tested. There may be other classic component features that are not covered by the existing tests though.

For example, you can pass a function via the didInsertElement (i.e. <Input @didInsertElement=... /> or {{input didInsertElement=...}}) and it would "work" by shadowing the method/hook on the class, because that's how arguments on classic components work. RFC #671 explained this in a bit more details.

We need to do an audit on the classic component API and find cases like this. In each case, we can decide:

  1. It is clearly not supported/it already throws an error today/it is clearly logically invalid (e.g. <Input @init=... /> – pretty sure it won't work because we require you to call super, or <Input @tagName=... /> – what do you want?!)
  2. It technically works but pretty sure nobody does that ever (I would guess <Input @didInsertElement=... /> falls into this area... I hope?!)
  3. It is very common

For the first 2 cases, we probably won't have to do anything.

For the third case, we'll probably have to emulate it and issue a deprecation message. #19218 has plenty of examples for this. In general, the strategy is to try an isolate the deprecated feature in a monkey patch outside of the class, so we can just delete it once the feature is no longer supported. If this is not possible or too difficult, we can also "deopt" the component (see the next section).

For cases that falls somewhere in between, we need to make a judgement call on what to do.

Implement the "deopt"

As mentioned above, there are some features that would be extremely difficult or impossible to emulate. RFC #671 gave some examples – such as calling .reopen on the component class.

The plan for this is to keep shipping the old classic component implementation and fallback ("deopt") to the old implementation when these features are used, with a loud deprecation warning of course. This can be accomplished by setting the modernized flag to false on the component.

There are two kinds of features that may want to trigger this deopt – static and non-static ones. Static features are something like Input.reopen(...). Once this is performed, we must deopt all Input components. The non-static features are when something passes an argument when invoking the component (say <Input @didInsertElement=... />), in which case we can just deopt that on instance only.

Something like this:

export default class Input extends InternalComponent {
  static modernized = Boolean(EMBER_MODERNIZED_BUILT_IN_COMPONENTS);

  static reopen(...args) {
    deprecate(...);
    this.modernized = false; // the static field
    return ClassicInput.reopen(...args);
  }

  modernized = this.constructor.modernized;
 
  constructor() {
    super(...arguments);

    if ('didInsertElement' in this.args) {
      deprecate(...);
      this.modernized = false; // the instance field
    }
  }

  ...
}

Submit the "part 2" deprecation RFC

The deprecations introduced in #19218 and any additional deprecations introduced from the work items above, as well as the equivalent deprecations for the <LinkTo> component, needs to go through the RFC process before they can be enabled. This is mentioned in RFC #671 as well.

We don't have to wait until we finished implementing all the deprecations to submit the RFC though. This can happen in parallel with the implementation, but it does require us to at least do the audit and think through what are the features we need to deprecate.

Migrate <Textarea> to be based on the new implementation

This is going to be very similar to the work in #19218. However, the functionality is almost identical so it can likely share most of the code with the Input component (it's sort of like a third @type). This can be either done with subclassing or extracting the common code.

Refactor <LinkTo>

This is again very similar to #19218, just for the <LinkTo> component instead.

eramod added a commit to eramod/ember.js that referenced this issue Feb 4, 2021
…ation

Tracking issue: emberjs#19270
More specifically, it enables the <Input> attribute deprecations introduced
in emberjs#19218, fixes broken tests, and
updates all test {{input}} invocations to use <Input> instead.

Deprecate input arguments
eramod added a commit to eramod/ember.js that referenced this issue Feb 4, 2021
…ation

Tracking issue: emberjs#19270
More specifically, it enables the <Input> attribute deprecations introduced
in emberjs#19218, fixes broken tests, and
updates all test {{input}} invocations to use <Input> instead.

Deprecate input arguments
chancancode added a commit that referenced this issue Feb 5, 2021
Instead of hard-coding a list of events to bind (copied from the
`EventDispatcher` – which could be reopened and modified), this
refactor uses a modifier to dynamically read the list from the
`EventDispatcher` at runtime instead.

Part of #19270
chancancode added a commit that referenced this issue Feb 5, 2021
Switches the <Textarea /> component to use the new implementation
as well. Passes all existing tests.

Part of #19270
chancancode added a commit that referenced this issue Feb 5, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode pushed a commit to eramod/ember.js that referenced this issue Feb 6, 2021
…ation

Tracking issue: emberjs#19270

More specifically, it enables the <Input> attribute deprecations introduced
in emberjs#19218, fixes broken tests, and
updates all test {{input}} invocations to use <Input> instead.
chancancode added a commit that referenced this issue Feb 6, 2021
Instead of hard-coding a list of events to bind (copied from the
`EventDispatcher` – which could be reopened and modified), this
refactor uses a modifier to dynamically read the list from the
`EventDispatcher` at runtime instead.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Switches the <Textarea /> component to use the new implementation
as well. Passes all existing tests.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Switches the <Textarea /> component to use the new implementation
as well. Passes all existing tests.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Switches the <Textarea /> component to use the new implementation
as well. Passes all existing tests.

Part of #19270
chancancode added a commit that referenced this issue Feb 6, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode added a commit that referenced this issue Feb 8, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode pushed a commit that referenced this issue Feb 13, 2021
…nt handlers

Tracking issue: #19270
Enables the input and textarea  event handlers deprecation introduced in #19218
and fixes resultant failing tests
chancancode added a commit that referenced this issue Feb 13, 2021
Refactor to extra shared code into internal component to pave way
for implementing <LinkTo> with the new infrastructure.

Part of #19270
chancancode added a commit that referenced this issue Feb 25, 2021
Instead of hard-coding a list of events to bind (copied from the
`EventDispatcher` – which could be reopened and modified), this
refactor uses a modifier to dynamically read the list from the
`EventDispatcher` at runtime instead.

Part of #19270
chancancode added a commit that referenced this issue Feb 25, 2021
Switches the <Textarea /> component to use the new implementation
as well. Passes all existing tests.

Part of #19270
chancancode added a commit that referenced this issue Feb 25, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of #19270
chancancode pushed a commit that referenced this issue Feb 25, 2021
…trings

Tracking issue: #19270
Get tests passing for no actions passed to inputs as strings deprecation
chancancode pushed a commit that referenced this issue Feb 25, 2021
…nt handlers

Tracking issue: #19270
Enables the input and textarea  event handlers deprecation introduced in #19218
and fixes resultant failing tests
chancancode added a commit that referenced this issue Feb 25, 2021
Refactor to extra shared code into internal component to pave way
for implementing <LinkTo> with the new infrastructure.

Part of #19270
@rwjblue
Copy link
Member

rwjblue commented Mar 3, 2021

@chancancode - Mind double checking that the right checkboxes are checked off?

sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
…ation

Tracking issue: emberjs#19270

More specifically, it enables the <Input> attribute deprecations introduced
in emberjs#19218, fixes broken tests, and
updates all test {{input}} invocations to use <Input> instead.
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
Instead of hard-coding a list of events to bind (copied from the
`EventDispatcher` – which could be reopened and modified), this
refactor uses a modifier to dynamically read the list from the
`EventDispatcher` at runtime instead.

Part of emberjs#19270
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
Switches the <Textarea /> component to use the new implementation
as well. Passes all existing tests.

Part of emberjs#19270
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
Deprecate all unsupported args (and "deopt" the component for now)
according to RFC 707.

Part of emberjs#19270
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
…trings

Tracking issue: emberjs#19270
Get tests passing for no actions passed to inputs as strings deprecation
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
…nt handlers

Tracking issue: emberjs#19270
Enables the input and textarea  event handlers deprecation introduced in emberjs#19218
and fixes resultant failing tests
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
Refactor to extra shared code into internal component to pave way
for implementing <LinkTo> with the new infrastructure.

Part of emberjs#19270
sly7-7 pushed a commit to sly7-7/ember.js that referenced this issue Mar 10, 2021
@locks
Copy link
Contributor

locks commented Jul 22, 2023

Beautiful work!

@locks locks closed this as completed Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants