Skip to content

[CLEANUP] Remove DEPRECATE_TEMPLATE_ACTION #20882

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

Merged
merged 8 commits into from
Mar 28, 2025

Conversation

tcjr
Copy link
Contributor

@tcjr tcjr commented Mar 22, 2025

Related to #20816.

The cleanup for this deprecation is pretty substantial, as it completely deletes two longstanding pieces of code from the resolver: the action modifier and the action helper. One consequence of this is that action is no longer a built-in keyword and can presumably be used now like any other helper and modifier.

@tcjr
Copy link
Contributor Author

tcjr commented Mar 24, 2025

@NullVoxPopuli @kategengler I restored the non-modifer parts of the modifier source file. All lints and tests and checks pass locally.

I am not sure how much of that very old event dispatcher code is in use anymore. Maybe "send" or some other old event mechanism still hits that code path.

@kategengler
Copy link
Member

While we deprecated and remove the action modifier and helper we didn't deprecate or remove the actions hash or the TargetActionSupport stuff https://github.com/emberjs/rfcs/blob/deprecate-target-action-support/text/0000-deprecate-target-action-support.md has some more info.

@tcjr
Copy link
Contributor Author

tcjr commented Mar 24, 2025

In a post-{{action}} modifier world, should we just remove this test?

  QUnit.test('Should not attempt to render element modifiers GH#14220', function (assert) {
    assert.expect(1);

    this.template('application', "<div {{action 'foo'}}></div>");

    return this.renderToHTML('/').then(function (html) {
      assert.htmlMatches(html, '<body><div></div></body>');
    });
  });

From app-boot-test.js

@NullVoxPopuli
Copy link
Contributor

In a post-{{action}} modifier world, should we just remove this test?

We could replace action with on

@tcjr
Copy link
Contributor Author

tcjr commented Mar 24, 2025

We could replace action with on

I believe this a FastBoot test that is verifying that a modifier that adds something to the DOM doesn't do it in the FastBoot context. For {{action}}, it adds a data- attribute. The on modifier doesn't modify the DOM (afaik), so I think we would need to create one for this test that changes the HTML.

Copy link

github-actions bot commented Mar 26, 2025

Development Assets

Diff

--- main/out.txt	2025-03-26 03:14:18.000000000 +0000
+++ pr/./pr-14129674314/out.txt	2025-03-28 15:59:38.000000000 +0000
@@ -1,4 +1,4 @@
- 2.3M └─┬ .
+ 2.2M └─┬ .
 1015K   ├─┬ @ember
  206K   │ ├─┬ -internals
   69K   │ │ ├─┬ views
@@ -14,7 +14,7 @@
   26K   │ │ ├─┬ meta
   21K   │ │ │ └── lib
   11K   │ │ ├── owner
- 9.9K   │ │ ├── deprecations
+ 9.6K   │ │ ├── deprecations
  7.4K   │ │ ├── metal
  7.0K   │ │ ├── string
  5.1K   │ │ ├── glimmer
@@ -63,7 +63,7 @@
  4.2K   │ ├── deprecated-features
  4.1K   │ ├── template-factory
  4.1K   │ └── version
- 729K   ├── shared-chunks
+ 709K   ├── shared-chunks
  384K   ├─┬ @glimmer
  166K   │ ├── runtime
   60K   │ ├── opcode-compiler

Details

This PRmain
Dev
 2.2M └─┬ .
1015K   ├─┬ @ember
 206K   │ ├─┬ -internals
  69K   │ │ ├─┬ views
  64K   │ │ │ └─┬ lib
  23K   │ │ │   ├── mixins
  22K   │ │ │   ├── system
  10K   │ │ │   ├── views
 4.3K   │ │ │   └── compat
  35K   │ │ ├─┬ runtime
  30K   │ │ │ └─┬ lib
  21K   │ │ │   ├── mixins
 5.7K   │ │ │   └── ext
  26K   │ │ ├─┬ meta
  21K   │ │ │ └── lib
  11K   │ │ ├── owner
 9.6K   │ │ ├── deprecations
 7.4K   │ │ ├── metal
 7.0K   │ │ ├── string
 5.1K   │ │ ├── glimmer
 4.9K   │ │ ├── utils
 4.9K   │ │ ├── routing
 4.5K   │ │ ├── error-handling
 4.5K   │ │ ├── utility-types
 4.2K   │ │ ├── container
 4.2K   │ │ ├── browser-environment
 4.1K   │ │ └── environment
 183K   │ ├─┬ routing
  28K   │ │ └── lib
 149K   │ ├─┬ object
  66K   │ │ └─┬ lib
  62K   │ │   └── computed
 114K   │ ├─┬ template-compiler
 109K   │ │ └─┬ lib
  20K   │ │   ├── plugins
 4.6K   │ │   ├── system
 4.1K   │ │   └── -internal
  66K   │ ├─┬ application
 5.6K   │ │ └── lib
  52K   │ ├─┬ debug
  21K   │ │ └── lib
  38K   │ ├─┬ array
 4.9K   │ │ └── lib
  31K   │ ├─┬ engine
 4.7K   │ │ └── lib
  27K   │ ├── runloop
  22K   │ ├─┬ utils
  18K   │ │ └── lib
  20K   │ ├── helper
  11K   │ ├── destroyable
 9.8K   │ ├── instrumentation
 9.4K   │ ├── controller
 7.4K   │ ├── service
 7.2K   │ ├── owner
 6.2K   │ ├── component
 5.6K   │ ├── canary-features
 5.5K   │ ├── modifier
 5.1K   │ ├── template-compilation
 5.0K   │ ├── enumerable
 5.0K   │ ├── test
 4.4K   │ ├── template
 4.4K   │ ├── renderer
 4.2K   │ ├── deprecated-features
 4.1K   │ ├── template-factory
 4.1K   │ └── version
 709K   ├── shared-chunks
 384K   ├─┬ @glimmer
 166K   │ ├── runtime
  60K   │ ├── opcode-compiler
  30K   │ ├── manager
  22K   │ ├── validator
  14K   │ ├── program
  12K   │ ├── reference
  11K   │ ├── destroyable
  10K   │ ├─┬ tracking
 4.4K   │ │ └── primitives
  10K   │ ├── util
 8.1K   │ ├── node
 7.3K   │ ├── global-context
 6.4K   │ ├── wire-format
 5.0K   │ ├── vm
 4.9K   │ ├── encoder
 4.6K   │ ├── owner
 4.1K   │ └── env
  60K   ├─┬ ember-testing
  56K   │ └─┬ lib
  14K   │   ├── test
  14K   │   ├── helpers
  10K   │   ├── ext
 6.5K   │   └── adapters
  31K   ├── backburner.js
  25K   ├── ember
  24K   ├── route-recognizer
  18K   ├─┬ @simple-dom
  14K   │ └── document
 9.2K   ├── dag-map
 4.3K   ├── rsvp
 4.3K   └── router_js
 2.3M └─┬ .
1015K   ├─┬ @ember
 206K   │ ├─┬ -internals
  69K   │ │ ├─┬ views
  64K   │ │ │ └─┬ lib
  23K   │ │ │   ├── mixins
  22K   │ │ │   ├── system
  10K   │ │ │   ├── views
 4.3K   │ │ │   └── compat
  35K   │ │ ├─┬ runtime
  30K   │ │ │ └─┬ lib
  21K   │ │ │   ├── mixins
 5.7K   │ │ │   └── ext
  26K   │ │ ├─┬ meta
  21K   │ │ │ └── lib
  11K   │ │ ├── owner
 9.9K   │ │ ├── deprecations
 7.4K   │ │ ├── metal
 7.0K   │ │ ├── string
 5.1K   │ │ ├── glimmer
 4.9K   │ │ ├── utils
 4.9K   │ │ ├── routing
 4.5K   │ │ ├── error-handling
 4.5K   │ │ ├── utility-types
 4.2K   │ │ ├── container
 4.2K   │ │ ├── browser-environment
 4.1K   │ │ └── environment
 183K   │ ├─┬ routing
  28K   │ │ └── lib
 149K   │ ├─┬ object
  66K   │ │ └─┬ lib
  62K   │ │   └── computed
 114K   │ ├─┬ template-compiler
 109K   │ │ └─┬ lib
  20K   │ │   ├── plugins
 4.6K   │ │   ├── system
 4.1K   │ │   └── -internal
  66K   │ ├─┬ application
 5.6K   │ │ └── lib
  52K   │ ├─┬ debug
  21K   │ │ └── lib
  38K   │ ├─┬ array
 4.9K   │ │ └── lib
  31K   │ ├─┬ engine
 4.7K   │ │ └── lib
  27K   │ ├── runloop
  22K   │ ├─┬ utils
  18K   │ │ └── lib
  20K   │ ├── helper
  11K   │ ├── destroyable
 9.8K   │ ├── instrumentation
 9.4K   │ ├── controller
 7.4K   │ ├── service
 7.2K   │ ├── owner
 6.2K   │ ├── component
 5.6K   │ ├── canary-features
 5.5K   │ ├── modifier
 5.1K   │ ├── template-compilation
 5.0K   │ ├── enumerable
 5.0K   │ ├── test
 4.4K   │ ├── template
 4.4K   │ ├── renderer
 4.2K   │ ├── deprecated-features
 4.1K   │ ├── template-factory
 4.1K   │ └── version
 729K   ├── shared-chunks
 384K   ├─┬ @glimmer
 166K   │ ├── runtime
  60K   │ ├── opcode-compiler
  30K   │ ├── manager
  22K   │ ├── validator
  14K   │ ├── program
  12K   │ ├── reference
  11K   │ ├── destroyable
  10K   │ ├─┬ tracking
 4.4K   │ │ └── primitives
  10K   │ ├── util
 8.1K   │ ├── node
 7.3K   │ ├── global-context
 6.4K   │ ├── wire-format
 5.0K   │ ├── vm
 4.9K   │ ├── encoder
 4.6K   │ ├── owner
 4.1K   │ └── env
  60K   ├─┬ ember-testing
  56K   │ └─┬ lib
  14K   │   ├── test
  14K   │   ├── helpers
  10K   │   ├── ext
 6.5K   │   └── adapters
  31K   ├── backburner.js
  25K   ├── ember
  24K   ├── route-recognizer
  18K   ├─┬ @simple-dom
  14K   │ └── document
 9.2K   ├── dag-map
 4.3K   ├── rsvp
 4.3K   └── router_js

@NullVoxPopuli
Copy link
Contributor

In a post-{{action}} modifier world, should we just remove this test?

maybe we just delete the test? it doesn't seem to be doing much. Then we can land this PR! 🎉

@tcjr
Copy link
Contributor Author

tcjr commented Mar 28, 2025

@kategengler This one looks ready to go. I removed the node (FastBoot) test that was using an {{action}} modifier.

@kategengler kategengler enabled auto-merge (squash) March 28, 2025 15:59
@kategengler
Copy link
Member

I've enabled auto-merge, thanks!

@kategengler kategengler merged commit c1c8b00 into emberjs:main Mar 28, 2025
24 checks passed
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