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

AS-406 Handle computed-property.override deprecation #76

Merged
merged 7 commits into from
Feb 20, 2024

Conversation

diegonakamashi
Copy link
Contributor

No description provided.

@diegonakamashi diegonakamashi self-assigned this Feb 15, 2024
@diegonakamashi diegonakamashi requested a review from a team as a code owner February 15, 2024 20:17
@diegonakamashi diegonakamashi requested review from marianoggf and removed request for a team February 15, 2024 20:17
Copy link

swarmia bot commented Feb 15, 2024

@diegonakamashi diegonakamashi force-pushed the AS-406/computed-property.override branch from 01a4b2c to 1effad1 Compare February 15, 2024 20:36
@@ -92,11 +92,19 @@ export default class PolarisDatePicker extends Component {
* @type {Boolean}
* @public
*/
@computed('selected')
@computed('selected', '_allowRange')
Copy link
Member

Choose a reason for hiding this comment

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

nit do we need to depend on _allowRange here? It's just used internally in this CP, or am I wrong?

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 added the "_allowRange" property here because when we set a new value for allowRange/_allowRange, I expected the locals using the get allowRange method to also be update dto the new value. Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do you mean this was added so any other property that depends on allowRange to be updated when allowRange changes?

With

@computed('<prop_name>')
get allowRange() {...}

you define on which properties it depends. So that for example, if <prop_name> changes, it will trigger updating allowRange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @vladucu 👋 , I understood what you mean and remove the property, but now I am getting this lint error:

[lint:js]   95:4  error  Use of undeclared dependencies in computed property: _allowRange  ember/require-computed-property-dependencies

Should I update the .lint-todo to ignore this error?

Copy link
Member

Choose a reason for hiding this comment

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

ahh, no...let's just leave that there then. thx for checking

Copy link
Contributor

@marianoggf marianoggf left a comment

Choose a reason for hiding this comment

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

Looks great so far

I found some cases with the overwritten issue:

  • shouldUseSearchEndpoint
  • colors-dropdown::ember4845>#options (branding -> theme)
  • smile-s3-uploader::ember5857>#fileProcessingOptions (branding -> emails & wallet)
  • There are a bunch in the Customer Emails page

There may be more but that's what I found

@diegonakamashi
Copy link
Contributor Author

Looks great so far

I found some cases with the overwritten issue:

  • shouldUseSearchEndpoint
  • colors-dropdown::ember4845>#options (branding -> theme)
  • smile-s3-uploader::ember5857>#fileProcessingOptions (branding -> emails & wallet)
  • There are a bunch in the Customer Emails page

There may be more but that's what I found

Hey @marianoggf 👋 ,
I think I address those cases on other PRs on smile-admin and ember-smile-core. ... But I will take a second look

@marianoggf
Copy link
Contributor

Just ignore me 😄

Copy link
Contributor

@marianoggf marianoggf left a comment

Choose a reason for hiding this comment

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

LGTM

@diegonakamashi diegonakamashi merged commit 1641e48 into main Feb 20, 2024
9 checks passed
@diegonakamashi diegonakamashi deleted the AS-406/computed-property.override branch February 20, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants