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

Ember.get/Ember.getWithDefault don't work as expected when the key name has "." in it since version 3.8 #18377

Closed
snemh opened this issue Sep 11, 2019 · 6 comments

Comments

@snemh
Copy link

snemh commented Sep 11, 2019

We have use case where the key name has "." in it.

Eg:
const unionSubContent = {
"id": 1,
"com.linkedin.campaignManager.subtreeNode": {
conversationId: 2
}
}
getWithDefault(unionSubContent, "com.linkedin.campaignManager.subtreeNode", {});

AND

{{leadgen-form-creation
data-test-defaultResponse={{question.typeSpecificQuestionDetails.[com.linkedin.voyager.feed.revenue.PreviewTextQuestionDetails].defaultResponse.string}}
}}

This worked as expected until 3.7. In 3.8 a commit was checked in to resolve path names correctly
cbbc4c4
This has stopped working since then.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 11, 2019

It looks like the original motivation for this was performance: #17166

I think given the perf improvements it's not really possible to revert that commit completely. It may be possible to do a check to see if that property exists in the object first before getting the path, but I feel like that would make the behavior of get a bit more ambiguous and confusing overall.

Short term, I would recommend converting to this:

const unionSubContent = {
  "id": 1,
  "com.linkedin.campaignManager.subtreeNode": {
    conversationId: 2
  }
}

let value = unionSubContent['com.linkedin.campaignManager.subtreeNode'] || {};

if you need to have more strict nullish defaulting behavior (e.g. only default if null/undefined), you could write a simple defaultTo function to help out:

function defaultTo(obj, key, defaultValue) {
  return (obj[key] === null || obj[key] === undefined) ? defaultValue : obj[key];
}

let value = defaultTo(unionSubContent, 'com.linkedin.campaignManager.subtreeNode', {});

In the future, nullish coalescing will make this much easier:

let value = unionSubContent['com.linkedin.campaignManager.subtreeNode'] ?? {};

Copy link
Member

rwjblue commented Sep 11, 2019

get and getWithDefault should have the same behaviors here, and both should be treating the argument as a property path (not a property name). The fix in 3.8 definitely seems correct to me...

@snemh
Copy link
Author

snemh commented Sep 11, 2019

Thanks for responding @pzuraq . Ya I already changed to use unionSubContent['com.linkedin.campaignManager.subtreeNode'] || {}; .

But given that the usage in template doesn't work as it used to work before, it may come as surprise to users.

Eg usage for template
{{leadgen-form-creation
defaultResponse={{question.typeSpecificQuestionDetails.[com.linkedin.voyager.feed.revenue.PreviewTextQuestionDetails].defaultResponse.string}}
}}

@rwjblue
Copy link
Member

rwjblue commented Sep 11, 2019

This does not seem like a valid template snippet, can you double check what you are using?

{{leadgen-form-creation
data-test-defaultResponse={{question.typeSpecificQuestionDetails.[com.linkedin.voyager.feed.revenue.PreviewTextQuestionDetails].defaultResponse.string}}
}}

@snemh
Copy link
Author

snemh commented Sep 11, 2019

Pasting from the real example

 {{#each formData.questions as |question idx|}}
      <div
      data-test-question-object={{idx}}
      data-test-question-id={{question.questionId}}
      data-test-question={{question.question}}
      data-test-name={{question.name}}
      data-test-predefinedField={{question.predefinedField}}
      data-test-responseEditable={{question.responseEditable}}
      data-test-defaultResponse={{question.typeSpecificQuestionDetails.[com.linkedin.voyager.feed.revenue.PreviewTextQuestionDetails].defaultResponse.string}}
      >
      {{#each question.typeSpecificQuestionDetails.[com.linkedin.voyager.feed.revenue.PreviewMultipleChoiceQuestionDetails].options as |option optionIdx|}}
        <div
          data-test-option-object={{optionIdx}}
          data-test-option-text={{option.[com.linkedin.voyager.feed.revenue.PreviewTextOptionQuestion].text}}
        />
      {{/each}}
      </div>
{{/each}}

@locks
Copy link
Contributor

locks commented Dec 4, 2020

getWithDefault is now deprecated and with the release of Octane, get is only necessary for Proxy object properties, so I'm not sure this is still relevant.
I'm closing but please re-open if necessary.

@locks locks closed this as completed Dec 4, 2020
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

No branches or pull requests

4 participants