Skip to content

docs(component-framework): setValue does not accept EntityReference instead it allows only a different format #5325

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mathis-m
Copy link

@mathis-m mathis-m commented May 13, 2024

The docs are currently stating that the method setValue is allowing EntityReference to be passed.
This is not the case. The current implementation only allows the format of:

type FixedEntityRef = {
    entityName: string;
    id: string;
    name: string;
};

This should be reflected in the docs. Or the implementation needs to be adjusted in order to support EntityReference.

Rel. DefinitelyTyped/DefinitelyTyped#63303 (comment)

…nstead it allows only the LookupValue format
@mathis-m mathis-m changed the title docs(component-framework): setValue does not accept EntityReference instead it allows only the LookupValue format docs(component-framework): setValue does not accept EntityReference instead it allows only the LookupValue type May 13, 2024
@mathis-m mathis-m changed the title docs(component-framework): setValue does not accept EntityReference instead it allows only the LookupValue type docs(component-framework): setValue does not accept EntityReference instead it allows only a different format May 13, 2024
Copy link
Contributor

Learn Build status updates of commit fad1aa2:

✅ Validation status: passed

File Status Preview URL Details
powerapps-docs/developer/component-framework/reference/entityrecord/setValue.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 678b94f:

✅ Validation status: passed

File Status Preview URL Details
powerapps-docs/developer/component-framework/reference/entityrecord/setValue.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@JimDaly
Copy link
Contributor

JimDaly commented May 13, 2024

@rarattay & @kaushikkaul

I'll try to summarize here. @mathis-m please correct me if I misunderstand.

The definition of EntityReference has these properties:

Name Type Description
etn string The table type name. Read-only.
id object The record id. Read-only.
The id object has a string guid property that contains the value of the ID
name string The name of the table reference. Read-only.

The change that @mathis-m proposes says that the EntityRecord.setValue method doesn't accept a value of this type.

However, I see no mention of an error or repro steps. Only an assertion that it doesn't work and a reference to a closed DefinitelyTyped PR from November of 2022.

@mathis-m proposes that an object with these properties works, and we should update the docs to specify that the setValue method use this rather than EntityReference:

Name Type Description
entityName string The table type name. Read-only.
id string The record id. Read-only.
name string The name of the table reference. Read-only.

@rarattay & @kaushikkaul

Please review this PR and comment about whether we should accept this change to the docs.

@JimDaly JimDaly added waiting-on-internal-feedback Topic owner is waiting on internal teams to respond before taking any action on the feedback do-not-merge labels May 13, 2024
@mathis-m
Copy link
Author

@JimDaly yes your summarization is correct.

I will post a reproduction example tomorrow. I can as well point you to the piece of code that does not behave as expected and where I got this format from. It will be minified but maybe it will help further investigations.

Maybe this issue should also be considered as product bug and not as a documentation issue, as this structure that is currently expected by the method, is not used anywhere else.

@mathis-m
Copy link
Author

mathis-m commented May 14, 2024

In the following bundle: app.1e9abfd54cb4766a61245bce0ee1a3b1.js / https://content.powerapps.com/resource/uci-infra-bus/scripts/app.1e9abfd54cb4766a61245bce0ee1a3b1.js

There is a function to produce the new values state, it handels all the different types of columns:
For the case of lookups:

            case a.a.Customer:
            case a.a.Owner:
            case a.a.Lookup:
            case a.a.Regarding:
                let t = null;
                return n instanceof i.a ? t = n : (n instanceof b.a || n && n.entityName && n.id && n.name) && (t = new i.a(n.entityName,new o.a(n.id),n.name)),
                {
                    reference: t,
                    timestamp: s,
                    validationResult: c
                };

Assuming n is our input. Lets call it instead valueToUpdate.
As we are passing an anonymous object the instanceof checks will not do much, so lets omit it:

let refrence = null;

if (valueToUpdate?.entityName  && valueToUpdate?.id && valueToUpdate?.name) {
   refrence = new Reference(valueToUpdate.entityName, new Guid(valueToUpdate.id), valueToUpdate.name)
}

return {
  reference: reference,
  timestamp: s,
  validationResult: c
};

As shown here the code will check for some instanceof checks, probably to check if it is already a reference that is used here internally, but not entierly sure about it.
If this not is the case the code checks that input object contains the following props entityName, id, name.

So if we pass in a different object structure, it will not throw or give any hint about the not matching structure it will just set the reference as null! This is also weird as this does not give the developer any chance to find an issue here.

Example:

// index.ts file of pcf
// type fixes

type RecordFix = EntityRecord & {
    /**
     * Whether this record is dirty. Only applicable if the dataset is editable and this record has dirty values.
     */
    isDirty(): boolean;

    /**
     * Saves the record.
     */
    save(): Promise<void>;

    /**
     * Set value for the column.
     * @param columnName Name of the column
     * @param value New value for the record
     */
    setValue(
        columnName: string,
        value:
            | string
            | Date
            | number
            | number[]
            | boolean
            | FixedEntityRef
            | FixedEntityRef[]
            | LookupValue
            | LookupValue[]
            | FileObject
    ): Promise<void>;
};

type FixedEntityRef = {
    entityName: string;
    id: string;
    name: string;
};

// ... Our test reference to set. this should be a valid owner ref:

const test: FixedEntityRef = {
  entityName: "systemuser",
  name: "Test, Testerson",
  id: "b466071f-843a-ed11-9db1-0022489fd45e"
}

// ... Inside the actual pcf class:

// Adding owner to dataset eg.
public init(context: ComponentFramework.Context<DefaultInputs>) {
  context.parameters.dataset.addColumn?.("owner");
  context.parameters.dataset.refresh();
}

public updateView(context: ComponentFramework.Context<DefaultInputs>) {
  const dataset = context.parameters.dataset;
  
  // Exit if owner column was not loaded
  if (!dataset.columns?.find((x) => x.name === "owner")) {
    return;
  }
  
  // For this test one record should exist in the loaded view :) otherwise we cannot reproduce the issue
  const record = dataset.records[dataset.sortedRecordIds[0]] as RecordFix | undefined;
  if (!record) {
    return;
  }
  
  const curVal = record.getValue("owner") as EntityReference;
  if (curVal.id.guid.includes("b466071f-843a-ed11-9db1-0022489fd45e")) {
    // If the break point is hit, it has worked
    debugger;
    return;
  }
  
  // Otherwise we will update the value to hit the break point to demonstrate that the undocumented format works:
  // This should trigger a rerender with the new value, even tho it was not saved. if it is not working maybe add await record.save() and dataset.refresh()
  record.setValue("owner", test);
}

If we instead pass an EntityReference it will not hit the breakpoint as it will always set the reference to null.

@JimDaly
Copy link
Contributor

JimDaly commented May 14, 2024

@mathis-m
Let's see what @kaushikkaul & @rarattay have to say.

I must admit, I don't follow your repro. Perhaps they will.
They must approve any change to the docs.

Sounds like you have a workaround that is working for you and you just want to see this updated in the docs so that others won't struggle with it. I appreciate that. But I also don't see evidence that others are struggling with this. Do you have any information to show others are encountering the same issue?

@mathis-m
Copy link
Author

@mathis-m

Let's see what @kaushikkaul & @rarattay have to say.

I must admit, I don't follow your repro. Perhaps they will.

They must approve any change to the docs.

Sounds like you have a workaround that is working for you and you just want to see this updated in the docs so that others won't struggle with it. I appreciate that. But I also don't see evidence that others are struggling with this. Do you have any information to show others are encountering the same issue?

Let's see what they have to say.

Regarding the evidence of others, take into consideration that this is a rarely used functionality, it is not even documented in the official typescript types. It is just in the wiki. There are only very limited blog posts or example online available, that cover the method setValue for simple types but not for lookup values.

Only thing that I can say after digging into the actual code that is running in dynamics, is that either the implementation or the wiki must change, as these are not compatible.

@phecke phecke removed their request for review July 12, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned-to-contributors do-not-merge waiting-on-internal-feedback Topic owner is waiting on internal teams to respond before taking any action on the feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants