KOGITO-2599: DataTable component API changes #332
Conversation
yzhao583
requested review from
Sara4994,
jstastny-cz,
pefernan,
AjayJagan and
cristianonicolai
| @@ -8277,9 +8277,9 @@ [email protected], escape-string-regexp@^1.0.2, escape-string-regexp@^1 | |||
| resolved "https://registry.yarnpkg.com/escape-string-regexp/-/escape-string-regexp-1.0.5.tgz#1b61c0562190a8dff6ae3bb2cf0200ca130b86d4" | |||
| integrity sha1-G2HAViGQqN/2rjuyzwIAyhMLhtQ= | |||
|
|
|||
| escodegen@^1.11.1: | |||
| escodegen@^1.11.1, escodegen@^1.8.1: | |||
There was a problem hiding this comment.
@yzhao583 I think this shouldn't be pushed...
There was a problem hiding this comment.
@pefernan Thanks for the review. I just installed the "jsonpath" package, don't know why so many dependencies got updated as well....
Are you suggesting not push local change of "yarn.lock"? Since I did not see it in the ".gitignore", so just want to confirm. Thanks
@cristianonicolai @AjayJagan @Sara4994
There was a problem hiding this comment.
It is a transitive dependency of jsonpath - this line specifies what versions were required either directly or transitively, wherease the decisive part is the line below - version "1.14.3" that's what actually got resolved as matching to all the "requirements" from above. And that version didn't change, so we're still on the same one.
But we should stick to a single registry url in yarn.lock imo - please run
yarn locktt --registry=https://registry.yarnpkg.com -s
More concerning is the esprima entry below - there we've added imo a second version of the same dependency.
There was a problem hiding this comment.
@jstastny-cz yea, I agree with the esprima.
@yzhao583 I know I already asked some changes on the design. But I think that, since the nested properties aren't required for now, we could rollback the that part and remove the jsonpath dep for now. I don't think we waht to have different dep versions.
There was a problem hiding this comment.
@jstastny-cz @pefernan If the change of "yarn.lock" in this PR is discarded, will the issue be fixed?
@pefernan I think the most important part of this PR is to support nested properties, if we decide to not have the feature for now, then I do not think it is necessary to have this PR opened.
There was a problem hiding this comment.
@pefernan @jstastny-cz I did some research about the issue, looks like we are not the only one to have the pain..and "esprima" is not the only dependency got duplicated. In our case, we have many dependencies got duplicated, such as "esutils", "eventemitter3", "extend-shallow", etc.
Looks like it is a issue has already been reported to yarn, here are some links to describe the issue:
yarnpkg/yarn#3778
https://medium.com/@scinos/de-duplicating-yarn-lock-ae30be4aa41a
Here are some solutions I found maybe useful:
https://medium.com/@bnaya/yarn-deduplicate-the-hero-we-need-f4497a362128
https://github.com/atlassian/yarn-deduplicate
In conclusion, this is definitely a issue that need our attention, but I would suggest to have a seperated PR to fix this, since it is related to so many dependencies and may need thoroughly test after the fix.
There was a problem hiding this comment.
@yzhao583 good point with https://github.com/atlassian/yarn-deduplicate.
I already seen that we are facing this issue of dupplicated deps with different versions
@jstastny-cz @cristianonicolai
To me I'd say that we should open a JIRA to fix that (. Seems like yarn deduplicate could solve the issue.) and continue with this PR.
There was a problem hiding this comment.
+1 to create a JIRA to look into yarn-deduplicate. That might actually be related to the issues we see when building on https://github.com/kiegroup/kogito-apps/pull/349.
I'm really just not sure how there two versions of "esprima" can coexist. @yzhao583 can you maybe that a look if we can safely use both?
There was a problem hiding this comment.
Hi everyone, I was brought here by @cristianonicolai's PR mention.
I want to reassure you about having multiple version of the same module in the lock file. It's not a big deal, actually that's quite common when the dependency list grows. If a package needs an older version of a module already installed, there's no other way then install it and add it to the lock (the esprima example).
It's yarn (npm) job to install different versions in the right place, nesting node_modules inside deps that have specific version requirements, so that they will resolve the correct one.
The downside is an increase in the bundle size. Besides that, there are generally no issues for transitive deps. There are some exceptions though, like with multiple version of react for example. But when eventual conflicts arise, they hardly go unnoticed.
Regarding dedupe, its purpose is a bit different. yarn already takes care of de-duplication in most of the cases. Some special treatment is required when multiple versions of a specific module are resolved even when only one of them could suffice according to listed versions inside package.json . It could happen sometimes, especially for projects with a long lifetime (the order in which deps are added matters) and a high number of deps. Again, I don't think we should worry about it until we face a specific case requiring dedupe.
The only problem I see, already fixed, was the presence of npm registry urls inside yarn.lock.
Sorry for the intrusion :)
There was a problem hiding this comment.
@yzhao583 thank you very much, the code looks well and works perfect. The only issue is the transitive dep. Since that change isn't a blocking requirement I may remove the jsonpath dep, and leave that part as it was before.
|
@yzhao583 could you please rebase? |
yzhao583
force-pushed
the
KOGITO-2599-DataTable-component-API-changes
branch
from
5847371
to
6d21170
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
* [BXMSPROD-2016] Updating references to the new kie-ci actions folder. * [BXMSPROD-2016] Updating references to the new kie-ci actions folder.
Jira: https://issues.redhat.com/browse/KOGITO-2599
Create a DataTableColumn interface to represent a column for our DataTable containing:
path (required): jsonPath to the object property we want to show, this will be used to get the row cells value.
label (required): String for the table header
bodyCellTransformer (optional): a functions to convert the cell content to a different format for a better visualization. The function should return either a string with the formatted content or either a React Component to be displayed on the Table cell. As parameters it will receive:
value: the actual cell value (calculated by the column path attribute)
rowDataObj: the full object rendered on the row.
Many thanks for submitting your Pull Request ❤️!
Please make sure that your PR meets the following requirements:
KOGITO-XYZ Subject[0.9.x] KOGITO-XYZ Subject