Skip to content

[FEATURE] Allow components in routes#20800

Merged
ef4 merged 1 commit intomainfrom
routable-components
Jan 7, 2025
Merged

[FEATURE] Allow components in routes#20800
ef4 merged 1 commit intomainfrom
routable-components

Conversation

@ef4
Copy link
Contributor

@ef4 ef4 commented Nov 22, 2024

Second attempt at #20768

  • Refactor to make component the primary path, normalizes templates into a custom component with {{this}} set to controller
  • Keep the core {{outlet}} responsibilities separate from what goes into the outlet
  • Ensures proper debug render tree output – route-template node only appears when a template (the custom component) is used
  • Ensures this works with test-helpers & others that
  • It doesn't differentiate between components and templates once normalized so @controller and @model is passed to both

TODO:

  • Feature flag
  • Test and integrate this in @ember/test-helpers + smoke test
  • Inlined some TODO comments to investigate/simplify things

Second attempt at #20768

* Refactor to make component the primary path, normalizes templates
  into a custom component with `{{this}}` set to controller
* Keep the core `{{outlet}}` responsibilities separate from what
  goes into the outlet
* Ensures proper debug render tree output – `route-template` node
  only appears when a template (the custom component) is used
* Ensures this works with test-helpers & others that
* It doesn't differentiate between components and templates once
  normalized so `@controller` and `@model` is passed to both

TODO:

* Feature flag
* Test and integrate this in @ember/test-helpers + smoke test
* Inlined some TODO comments to investigate/simplify things

Co-authored-by: Edward Faulkner <edward@eaf4.com>
@ef4
Copy link
Contributor Author

ef4 commented Nov 22, 2024

(This was reworked by @chancancode, I just opened the PR.)

@ef4
Copy link
Contributor Author

ef4 commented Nov 22, 2024

@chancancode what is the detail on the todo item "integrate this in @ember/test-helpers + smoke test"?

The PR does contain an end-to-end smoke test already, and I don't know what would need to change in @ember/test-helpers related to this.

@chancancode
Copy link
Member

chancancode commented Nov 22, 2024 via email

@ef4
Copy link
Contributor Author

ef4 commented Dec 6, 2024

My understanding of this PR currently is that it's trying to not break the intimate API that @ember/test-helpers uses, but that we should add test coverage to ensure that is true.

@chancancode
Copy link
Member

Not quite, I think we do have tests that tries to roughly cover that already, and indeed I don't think anything I changed would have broken that: https://github.com/emberjs/ember.js/blob/main/packages/ember/tests/ember-test-helpers-test.js

What I am saying was that, one key change I made here over the original PR was that I moved the normalization logic outside of the routing layer (when we resolve the route templates) and pushed it into the rendering layer (where we render the outlet).

One reason that is an improvement is because ("things like") ember-test-helpers may bypasses the routing code and set the outlet state directly. So the refactor I did ensures that would also work.

To that end I think it's good to have some test exercising that code path, confirming setting the outlet state directly with a component actually does work. One way to do that is add another module in the ember-test-helpers smoke test and speculatively change to what we think ember-test-helpers should do moving forward. An artificial unit test somewhere else may also work, but I don't think it'd be that much simpler.

@ef4
Copy link
Contributor Author

ef4 commented Dec 6, 2024

Got it, thanks.

@ef4
Copy link
Contributor Author

ef4 commented Jan 7, 2025

Merging this, because in order to truly exercise making ember-test-helpers take advantage of this "new way" it will be easier to do so against an actual ember version that support the feature. We expect everything to already work, and the change in ember-test-helpers is purely cleanup-refactoring, not required for correctness.

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.

2 participants