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

Linked Target Properties #61

Merged
merged 9 commits into from Jan 18, 2018
Merged

Linked Target Properties #61

merged 9 commits into from Jan 18, 2018

Conversation

@javan
Copy link
Contributor

@javan javan commented Jan 17, 2018

Define a controller's target names and Stimulus automatically creates properties for accessing them:

import { Controller } from "stimulus"

export default class extends Controller {
  static targets = [ "source", "slide" ]

  initialize() {
    this.sourceTarget  // this.targets.find("source")
    this.sourceTargets // this.targets.findAll("source")

    this.slideTarget  // this.targets.find("slide")
    this.slideTargets // this.targets.findAll("slide")
  }
}
@javan javan added this to the 1.0 milestone Jan 17, 2018
@sstephenson
Copy link
Contributor

@sstephenson sstephenson commented Jan 17, 2018

https://github.com/stimulusjs/stimulus/blob/ca4f4dcd2de7d9905c7864d52d2497670e15e69a/handbook/02_hello_stimulus.md#L150-L165

Let’s take this change all the way. No need to start by explaining this.targets—we can reserve that API for special cases. We should revise the whole section to introduce targets as properties right off the bat.

@javan
Copy link
Contributor Author

@javan javan commented Jan 17, 2018

Good call. Made some revisions in af233fe

@sstephenson
Copy link
Contributor

@sstephenson sstephenson commented Jan 17, 2018

Much better! Probably need to revisit the title and intro, too:

https://github.com/stimulusjs/stimulus/blob/af233fe50df6b7c799c28c372319ec7ef4554213/handbook/02_hello_stimulus.md#L129-L135

“Locate” now seems to describe the implementation rather than the concept. And we aren’t really “referencing” elements “by name” anymore, either. I would prefer to describe it more along the lines of linking targets to controller properties.

@sstephenson
Copy link
Contributor

@sstephenson sstephenson commented Jan 17, 2018

I’m happy with chapter 2 of the handbook now. The last thing we should do is update the installation guide to mention the necessary Babel static properties plugin.

@javan
Copy link
Contributor Author

@javan javan commented Jan 18, 2018

It turns out that the Babel plugin for class properties is either going to work great or not work at all with our implementation.

👍

Add the transform-class-properties plugin to your .babelrc file:

{
  "plugins": ["transform-class-properties"]
}

And classes with static properties like:

class A {
  static targets = []
}

Are compiled to:

var A = function() {}
A.targets = []

👎

Use the plugin's spec: true option like webpacker does:

{
  "plugins": ["transform-class-properties", { "spec": true }]
}

And the same class compiles to:

var A = function() {}
Object.defineProperty(A, "targets", { value: [] })

Thus bypassing our setter:
https://github.com/stimulusjs/stimulus/blob/1fcfa4f1567b9d918d9163c662dc67f364f3b296/packages/%40stimulus/core/src/controller.ts#L11-L14

In Babel 7, this is going to be the default behavior unless you use the loose: true option.

@dixpac
Copy link

@dixpac dixpac commented Jan 18, 2018

Thinking out loud...is it possible to get target properties without specifying them explicitly inside the controller?

// src/controllers/hello_controller.js
import { Controller } from "stimulus"

export default class extends Controller {
  //static targets = [ "name" ]       without this line

  greet() {
    const element = this.nameTarget
    const name = element.value
    console.log(`Hello, ${name}!`)
  }
}

@javan
Copy link
Contributor Author

@javan javan commented Jan 18, 2018

@dixpac we could with Proxy, but it's unsupported in IE and can't be polyfilled.

To avoid the potential pitfalls with Babel outlined in #61 (comment)
@javan
Copy link
Contributor Author

@javan javan commented Jan 18, 2018

Possible solution to the Babel situation: c8e7696

@javan javan force-pushed the target-definitions branch from e3fc4df to 88bfde8 Jan 18, 2018
@javan javan merged commit 5bcae2f into 1-0 Jan 18, 2018
1 check passed
@javan javan deleted the target-definitions branch Jan 18, 2018
@javan javan changed the title Target definitions Linked Target Properties Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants