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

Expose root as a public property #1023

Merged
merged 1 commit into from
Dec 13, 2017
Merged

Expose root as a public property #1023

merged 1 commit into from
Dec 13, 2017

Conversation

emilos
Copy link
Contributor

@emilos emilos commented Dec 13, 2017

Hey,

I'm opening the PR to discuss the root property being public (it was briefly discussed in gitter and I'll try to describe the motivation here once again).

I have an use case where I'd like to create a new instance of a component with an additional service property. The main motivation is easier testing with a fake service, secondary motivation is to make the chat be less dependent on a particular vendor.

const service = new VendorService() // handles http/ws communication, the chat is not tied to a particular backend, it'll work for any as long as this service has right methods/events

new Chat({
   target: document.getElementById('foo'),
   data: { brand: 'Bar Baz' },
   service
})

the chat component itself looks like this:

{{#if closed}}
<Bar on:click='toggle(OPEN)'>Leave a message</Bar>
{{/if}}

{{#if open}}
<Widget on:click='toggle(CLOSE)'/>
{{/if}}

then inside of the Widget I use the service e.g. to send a message, receive messages etc.:

export default {
  oncreate () {
    this.service = this._root.options.service
  },
  methods () {
    send (event) {
      // set data etc.
      this.service.emit('send:message', message.text)
    }
  }
}

other approaches I though of or that were suggested to solve this:

a) propagate events from lowest components (e.g. form submits) and handle all of it outside of the chat component - I didn't like it because I'd need to propagate it via many layers, too much hassle, too close to redux kind of way which I find very verbose

b) check if service can be passed like the store, so that it can be automatically injected into components below - based on discussion in gitter I understood that it seems that dependency injection was discussed before and it's seen as a fragile pattern (I have the same feeling based on what I saw in other libraries, however I also think that it's not necessarily the pattern that is wrong but the excessive use, any pattern is wrong when overused)

new Chat({
  target: document.getElementById('foo'),
  store,
  includes: { service }
})

then the line:

this.service = this.root.options.service

is unnecessary.

c) pass it via store as store.service. A little bit weird, unexpected

d) pass it via data. Weird as well, unexpected

I do understand the desire to keep svelte as concise as possible so I'm open to all suggestions :) Thanks a lot.

@codecov-io
Copy link

codecov-io commented Dec 13, 2017

Codecov Report

Merging #1023 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1023   +/-   ##
======================================
  Coverage    92.1%   92.1%           
======================================
  Files         115     115           
  Lines        4304    4304           
  Branches     1373    1373           
======================================
  Hits         3964    3964           
  Misses        147     147           
  Partials      193     193
Impacted Files Coverage Δ
src/generators/dom/index.ts 95.37% <ø> (ø) ⬆️
src/generators/nodes/Element.ts 94.88% <ø> (ø) ⬆️
src/generators/nodes/Component.ts 95.81% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c86487...fadeeaf. Read the comment docs.

@Rich-Harris
Copy link
Member

Thanks! I think this is probably a good compromise — it basically enables all the things you'd want to use a more fully-fledged DI system for, but it doesn't 'encourage' it, and it doesn't cost anything (in fact we're just removing _ bytes!).

I'll leave this open for a short while to see if anyone else has thoughts and wants to weigh in.

@Conduitry
Copy link
Member

Conduitry commented Dec 13, 2017

Might we also want this.root === this on the root component itself? It doesn't look like that's the case currently.

Edit - Whoops nevermind, I'm dumb. It is. Carry on.

@Rich-Harris
Copy link
Member

This is released as 1.48.0 — thanks

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