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

Add a section for using vanilla classes with dependency injection #1974

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Nov 2, 2023

I noticed this was missing, and it's hugely important and powerful.
(and our discord api bot didn't find anything useful when I asked, here: https://discord.com/channels/480462759797063690/1157084708442755102/1169635166390587493 )

As I was writing this though, I was realizing that the guides need a whole section on managing reactivity, how to think about lazy evaluation in an auto-tracked world, etc.

I've started brainstorming topics over here: NullVoxPopuli/website#117
and I'll more than likely write something over on my blog before writing something for the guides -- but it's just a big gap I noticed as I was trying to explain concepts in this PR.

If I missed something please lemme know!


Related content that I can never find through search: #1715

Copy link

netlify bot commented Nov 2, 2023

Deploy Preview for ember-guides ready!

Name Link
🔨 Latest commit 5a1ee2e
🔍 Latest deploy log https://app.netlify.com/sites/ember-guides/deploys/6543e037ed082800083c56b5
😎 Deploy Preview https://deploy-preview-1974--ember-guides.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@lukasnys lukasnys left a comment

Choose a reason for hiding this comment

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

Some minor remarks, feel free to not apply the suggestions if you feel they might not make sense!

guides/release/services/index.md Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
guides/release/services/index.md Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
guides/release/services/index.md Outdated Show resolved Hide resolved
@lukasnys
Copy link
Contributor

lukasnys commented Nov 2, 2023

Thanks for writing these docs! ❤️

NullVoxPopuli and others added 7 commits November 2, 2023 12:00
Co-authored-by: Lukas Nys <nyslukas@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>

#### With arguments

If your native class needs arguments, we can change the above example to instantiate the class like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

From here down seems like useful content, but maybe unrelated to services. Not sure where else it could go. Do we have a cookbook section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it's more related with "how to in general work with native classes in general reactive systems" -- which we don't have a section for about reactivity patterns, derived data, or anything like that.

there was a whole cookbook project, rfc and such, I don't have context as to where that's at tho

NullVoxPopuli and others added 3 commits November 2, 2023 13:42
Co-authored-by: Krystan HuffMenne <kmenne+github@gmail.com>
Co-authored-by: Krystan HuffMenne <kmenne+github@gmail.com>
Co-authored-by: Lukas Nys <nyslukas@gmail.com>

export class VanillaClass {
@service shoppingCart;

Copy link
Contributor

Choose a reason for hiding this comment

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

If the class is going to use a service, it really should have the owner passed in to the constructor and do the setOwner in the constructor like glimmer components imo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really agree :-\

if the responsibility of setting up the owner is always in the constructor then it's impossible to abstract reasonably, as one might to do here: https://ember-resources.pages.dev/funcs/link.link

```

With this technique, the tracked data provided by `this.arg.foo` is lazily evaluated in `VanillaClass`, allowing the `VanillaClass` to participate in lazy evaluation and auto-tracking like every where else you may be used to in an app.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t love these suggestions here. These are the problems I was bringing up in the spec channel in discord the other day. We can discuss more in the meeting today

Copy link
Contributor Author

@NullVoxPopuli NullVoxPopuli Nov 2, 2023

Choose a reason for hiding this comment

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

we only have two ways to pass reactive data (in js) without consuming it:

  • wrapper class/object/whatever (with getters)
  • arrow function

curious what solutions you think there may be

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.

4 participants