mapStateToCellProps does not translate error messages

[codefactor]

Describe the bug

When creating custom Cells, the errors prop is always the untranslated message, and not the value returned by the translateError function of our JsonFormsI18nState object.

Expected behavior

The errors passed by mapStateToCellProps should be passed through translateError to get project-level translations.

Steps to reproduce the issue

1. Create your own cell renderer that expects to get the error message
2. Create a form that uses the cell, and define a custom translateError function
3. Check the error message that the cell renderer gets
4. See that the cell does not get the translations from the translateError

Screenshots

I don't have any screenshots.

In which browser are you experiencing the issue?

any

Which Version of JSON Forms are you using?

v3.0.0

Framework

Core

RendererSet

Other (please specify in the Additional context field)

Additional context

We're creating our own custom renderers, where the low-level input controls need the error message. For an example of the input control itself see the following:
https://sap.github.io/ui5-webcomponents-react/?path=/docs/inputs-input--default-story

This component uses a <ui5-input> web component which has a valueStateMessage field to specify the error message that only displays once you focus into the input and shows as a tooltip on the control.

We've created a custom TextCell that uses the above Input control internally; however, we've noticed that none of the messages that show up here are ever translated. Upon investigation this appears to be a bug at the @jsonforms/core layer here:

jsonforms/packages/core/src/util/cell.ts

Lines 151 to 153 in 1b22f55

	const errors = formatErrorMessage(
	union(getErrorAt(path, schema)(state).map(error => error.message))
	);

The only solution/workaround appears to have our TextCell to formulate the error message by directly calling mapStateToControlProps after pulling the ctx object which is not a clean solution. I suspect that with typical renderers the Cell components don't read the errors prop, so this may never have been brought up before.

[codefactor]

https://codesandbox.io/s/romantic-lederberg-63c4td?file=/src/App.jsx

In the above simple/contrived example, I've created 2 custom renderers - one for a TextCell and one for a TextControl, the renderer simply outputs the props.errors string so that it's visible on the UI. The <JsonForms> is passed a simple translate function and translateError function to wrap the default translation with a simple string so that it's obvious whether or not the custom translate function is used to translate a message.

We see in the red box that the text is not being passed through translation, and that we only get the default developer-only english message. But the green box we see the message is wrapped so it's working as expected.

[codefactor]

Here is a gist for a function to wrap my custom Cells with to get a translated error message:
https://gist.github.com/codefactor/7fb0c13816b49555f6e9b2a2c23b78fd

To use this solution the Cell

// BEFORE
const CustomTextCell = withJsonFormsCellProps(
  withVanillaCellProps(CustomTextCellRenderer)
);

// AFTER
const CustomTextCell = withJsonFormsCellPropsWithI18nError(
  withVanillaCellProps(CustomTextCellRenderer)
);

We can consider the above a kind of hack because it requires the Wrapper logic to do duplicate logic that the mapStateToCellProps has already done.

I believe the true fix here is to call getCombinedErrorMessage from cell.ts:

jsonforms/packages/core/src/util/cell.ts

Lines 151 to 153 in 1b22f55

	const errors = formatErrorMessage(
	union(getErrorAt(path, schema)(state).map(error => error.message))
	);

  const t = getTranslator()(state);
  const te = getErrorTranslator()(state);
  const errors = getCombinedErrorMessage(errors, te, t, schema, uischema, path);

But I guess that the authors here have purposely done this for some reason, and I guess it's because most Cell type renderers don't even read the useless errors string - since it won't be translated I'm not sure why it's even there?

[sdirix]

This was discussed in our forum.

In summary: The mapStateToCellProps should invoke the i18n feature too when handing over errors.

[codefactor]

Probably later today I can propose a PR for this, as the fix is already documented here it should be straight forward.

[codefactor]

@sdirix ,
I am not 100% sure how to run the unit tests - but I tried my best to write a test - please see the following PR:
#2098

I looked through other unit tests for cell.ts and I can't find any unit tests that were testing for errors, so I am hoping that the unit tests will pass and I only need to add one more unit test (rather than modifying any other test that could potentially fail due to this change).

Any assistance here to get me started running a test?

Here's what I did so far:

1. tried to run npm install - get failure about old lock file, so I killed the install process quickly and used nvm to downgrade to version 14 of node (which is out of maintenance in a few months, but I guess you guys still use it... you should probably think to upgrade?)
2. ran npm install again on the root project and then npm run init - all from root project which runs without any errors now (note that I didn't let the node 16 finsih npm install so the package-lock was clean)

I am getting errors like this when running npm run test now from the root:

> @jsonforms/core@3.1.0-alpha.1 test /Users/i832513/SAPDevelop/xweb/jsonforms/packages/core
> cross-env TS_NODE_COMPILER_OPTIONS={\"module\":\"commonjs\",\"target\":\"es5\"} ava


  Uncaught exception in test/generators/uischema.test.ts

  /Users/i832513/SAPDevelop/xweb/jsonforms/node_modules/ts-node/src/util.ts:58

  SyntaxError: Unexpected token a in JSON at position 1

[sdirix]

Thanks for the PR!

npm ci && npm run init && npm run build && npm run test is usually the way to go. However we have some problems with tests on Mac at the moment which is why they are disabled there in the CI too. The tests will be executed by our CI on Linux and Windows.

you should probably think to upgrade?)

It's a high priority item for the near future 👍

[codefactor]

@sdirix ,
I only have a Mac to run the unit tests. How do mac users like myself run the tests locally?

[codefactor]

@sdirix ,
I tried with a Dockerfile like this:

FROM node:14
COPY . .
RUN npm ci && npm run init && npm run build && npm run test

Then ran like this:

docker build -t jsonforms-test:0.0.1-SNAPSHOT .

And the build got further this time but failed:

#5 568.5   ✔ util › renderer.ts › mapStateToLabelProps - i18n - default translation has no effect
#5 568.5   ✔ util › renderer.ts › mapStateToLabelProps - i18n - default key translation
#5 568.5   ✔ util › renderer.ts › mapStateToLabelProps - i18n - default message translation
#5 568.5   ✔ util › renderer.ts › mapStateToLabelProps - i18n - i18n key translation
#5 568.5   ✖ test/util/cell.test.ts exited due to SIGABRT
#5 568.5 
#5 568.5   310 tests passed
#5 568.5   1 known failure
#5 568.5 
#5 568.5   generators › schema.ts › default schema generation tuple array types
#5 568.5 
#5 568.5 
#5 568.5 lerna ERR! npm run test stderr:
#5 568.5 
#5 568.5 <--- Last few GCs --->
#5 568.5 
#5 568.5 [1777:0x5071fe0]   142927 ms: Mark-sweep (reduce) 2045.7 (2050.6) -> 2044.7 (2051.6) MB, 941.3 / 0.0 ms  (average mu = 0.120, current mu = 0.003) allocation failure scavenge might not succeed
#5 568.5 [1777:0x5071fe0]   143874 ms: Mark-sweep (reduce) 2045.7 (2050.6) -> 2044.8 (2051.6) MB, 943.8 / 0.0 ms  (average mu = 0.065, current mu = 0.003) allocation failure scavenge might not succeed
#5 568.5 
#5 568.5 
#5 568.5 <--- JS stacktrace --->
#5 568.5 
#5 568.5 FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
#5 568.5  1: 0xa3ad50 node::Abort() [/usr/local/bin/node]
#5 568.5  2: 0x970199 node::FatalError(char const*, char const*) [/usr/local/bin/node]
#5 568.5  3: 0xbba90e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node]
#5 568.5  4: 0xbbac87 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [/usr/local/bin/node]
#5 568.5  5: 0xd76ea5  [/usr/local/bin/node]
#5 568.5  6: 0xd77a2f  [/usr/local/bin/node]
#5 568.5  7: 0xd8586b v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [/usr/local/bin/node]

Since it failed on the cell.test.ts - I guess it could be a coincidence or maybe it's my code that caused it to fail.

[sdirix]

Looks like an actual error as it also occurs in our CI.

[codefactor]

@sdirix ,
Thanks, that's what I suspected -- can you help me to understand the workflow for a mac user to run the tests to debug a problem such as this?

[sdirix]

It seems you were able to reproduce the problems in a docker container, right? So in that case you can simply use our VS Code Dev Container

[codefactor]

@sdirix ,

Thanks, I figured out how to make my container approach re-run the tests without npm install every time, which was my issue before. I will consider giving the VS Code Dev Container a try if I do more contributions to this project.

[sdirix]

Fixed with #2098