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

Make actions execute with the component context #1279

Merged
merged 2 commits into from
Mar 27, 2018

Conversation

jacwright
Copy link
Contributor

Actions have an undefined this. This PR makes this be the component.

They were only passing when running just the runtime tests, but failing with `<button>undefined</button>` when running all the tests.
@codecov-io
Copy link

codecov-io commented Mar 26, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1279   +/-   ##
=======================================
  Coverage   91.51%   91.51%           
=======================================
  Files         121      121           
  Lines        4326     4326           
  Branches     1361     1361           
=======================================
  Hits         3959     3959           
  Misses        153      153           
  Partials      214      214
Impacted Files Coverage Δ
src/generators/nodes/Element.ts 95.15% <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 fcfbffe...297ee65. Read the comment docs.

@ekhaled
Copy link
Contributor

ekhaled commented Mar 26, 2018

Excellent work on this @jacwright

I have a small itch that I need to scratch.

Ractive used to have the concept of keypath.
A decorator function would accept node and keypath, and within the function body update the keypath by calling this.set(keypath, newValue).

Here, although we have access to the whole component instance, it's impossible to know exactly which key to update. This makes it harder to write general-purpose decorators.

Let me illustrate with an example:

<!--Component.html-->
<input type="text" use:calendarAction="startDate" />
<input type="text" use:calendarAction="endDate" />
<script>
import calendarAction from './calendarAction.js';

export default {
  data(){
    return { startDate:'', endDate:'' }
  },
  actions:{ calendarAction }
}
</script>
//calendarAction.js
import externalCalendar from 'someCalendar.js';
export default function(node, data){
  
  //wire up click event to show calendar
  //when calendar date is clicked, which key do I update?
 
  return {
   update(data){},
   destroy(){}
  }
}

I hope the above made sense.
As you can see, it's impossible for that action to serve both <input>s

@ekhaled
Copy link
Contributor

ekhaled commented Mar 26, 2018

Hmm, may have found a workaround, but involves sending strings in data and treating them like keypaths in ractive.

<!--Component.html-->
<input type="text" use:calendarAction="'startDate'" />
<input type="text" use:calendarAction="'endDate'" />
<script>
import calendarAction from './calendarAction.js';

export default {
  data(){
    return { startDate:'', endDate:'' }
  },
  actions:{ calendarAction }
}
</script>
//calendarAction.js
import externalCalendar from 'someCalendar.js';
export default function(node, key){
  
  //here data is actually a key
  //component's data object

  var self = this,
  updating = false,
  updateObject = {};
  
  var calendar = new externalCalendar({
    onSelect:function(new_date){
      //not from a real calendar framework
      updateObject[key] = new_date;
      updating = true;
      self.set(updateObject);
      updating = false;
    }
  });

  var observer = self.observe(key, function(date_from_elsewhere){
    //bind external updates back to calendar
    if(updating) return;
    calendar.setDate(date_from_elsewhere)
  });
 
  return {
    update(data){// this is moot},
    destroy(){
      calendar.destroy();
      observer.cancel();
      //and reomve other events etc.
    }
  }
}

@jacwright
Copy link
Contributor Author

jacwright commented Mar 26, 2018

@ekhaled I see what you mean. Without having keyPath you would have to do something like this:

<!--Component.html-->
<input type="text" use:calendarAction="{ key: 'startDate', value: startDate }" />
<input type="text" use:calendarAction="{ key: 'endDate', value: endDate }" />
<script>
import calendarAction from './calendarAction.js';

export default {
  actions:{ calendarAction }
}
</script>

This works, but it isn't as clean. I'm unsure how we might go about providing this since actions can accept functions and literals. e.g. use:tooltip="$t('trans_key')" or use:foo="{ prop: value }". Thoughts @Rich-Harris?

@ekhaled
Copy link
Contributor

ekhaled commented Mar 26, 2018

I suppose my use case is quite edge, and can be achieved using the string workaround I mentioned. As we can use both get and set from the component instance.

Usually this sort of thing would be achieved using a component.

@Rich-Harris Rich-Harris merged commit c9435fc into sveltejs:master Mar 27, 2018
@Rich-Harris
Copy link
Member

nice, thanks @jacwright 👌

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.

None yet

4 participants