Skip to content

Remove remaining traces of wrapComputed.#152

Merged
pzuraq merged 5 commits intoember-codemods:masterfrom
rwjblue:remove-vestiges-of-wrap-computed
Aug 14, 2019
Merged

Remove remaining traces of wrapComputed.#152
pzuraq merged 5 commits intoember-codemods:masterfrom
rwjblue:remove-vestiges-of-wrap-computed

Conversation

@rwjblue
Copy link
Copy Markdown
Member

@rwjblue rwjblue commented Jun 24, 2019

  • Moves setRuntimeData before setDecorators so that we can more easily setup local state in setDecorators
  • Removes remaining references to wrapComputed * Adds .isComputed boolean to EOProp class to allow us to track when the runtime data indicates that a given property was a computed property.
  • Update decorator building code to avoid creating a call expression for decorators that are returning computed properties without arguments (e.g. use @someComputedMacro vs @someComputedMacro())

First step in addressing #142 and #144

@pzuraq
Copy link
Copy Markdown
Collaborator

pzuraq commented Jun 24, 2019

Update decorator building code to avoid creating a call expression for decorators that are returning computed properties without arguments (e.g. use @someComputedMacro vs @someComputedMacro())

This is likely not correct, you would need to convert the macro function to detect whether or not it is being called as a decorator also.

// this function cannot be called as a decorator
function someComputedMacro() {
  // the function this `computed` returns can
  return computed();
}

Unless someComputedMacro is a computed property, it should be still be called.


const decoratorExpr = instanceProp.modifiers.reduce(
let decoratorExpression =
['computed', 'service', 'controller'].includes(decoratorName) && decoratorArgs.length === 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we check against hard coded decorator names? Would it work in case they are imported with different local name? For example service is imported as inject or some other local name

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The deocratorName here is the name that it was imported from not what is named in the module. I'll add a specific test case for that.

decoratorExpression
);

if (!instanceProp.modifiers.length) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can move this before the reduce statement above

import Service, { service as injectService } from '@ember/service';

export default Service.extend({
something: service(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be something: injectService()?


export default Service.extend({
something: service(),
otherThing: service('some-thing')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here

Robert Jackson and others added 5 commits August 12, 2019 09:52
* Moves `setRuntimeData` _before_ `setDecorators` so that we can more
  easily setup local state in `setDecorators`
* Removes remaining references to `wrapComputed`
* Adds `.isComputed` boolean to `EOProp` class to allow us to track when
  the runtime data indicates that a given property was a computed
  property.
* Update decorator building code to avoid creating a call expression for
  decorators that are returning computed properties without arguments
  (e.g. use `@someComputedMacro` vs `@someComputedMacro()`)
@pzuraq pzuraq force-pushed the remove-vestiges-of-wrap-computed branch from f14077e to 3e7962a Compare August 14, 2019 21:42
@pzuraq
Copy link
Copy Markdown
Collaborator

pzuraq commented Aug 14, 2019

Going to merge this now that tests are green and keep working, there's a fair amount more to do/cleanup in general

@pzuraq pzuraq merged commit c1a0eff into ember-codemods:master Aug 14, 2019
@rwjblue rwjblue deleted the remove-vestiges-of-wrap-computed branch August 20, 2019 14:56
@rwjblue rwjblue added the bug Something isn't working label Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants