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

Support code references at any level within extension's properties #9070

Merged
merged 3 commits into from Aug 10, 2021

Conversation

vojtechszocs
Copy link
Contributor

@vojtechszocs vojtechszocs commented May 27, 2021

With this PR, CodeRef<T> values can appear at any level within extension's properties object.

For example, using the following Console extension declaration

export type Foo = ExtensionDeclaration<'console.foo', {
  bar: CodeRef<VoidFunction>;
  baz: { qux: CodeRef<VoidFunction> };
}>;

both e.properties.bar and e.properties.baz.qux values will be

  1. parsed (EncodedCodeRef to CodeRef) when loading the given plugin
  2. resolved (CodeRef<T> to T) when consumed via useResolvedExtensions hook

Detection of code references (both encoded & executable) relies on a recursive equivalent of _.forOwn function that traverses plain objects and arrays. This means that e.g. CodeRef<VoidFunction>[] (array of code references) properties are supported as well.

cc @bipuladh @christianvogt

@openshift-ci openshift-ci bot added component/shared Related to console-shared approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 27, 2021
export const mergeExtensionProperties = <E extends Extension>(e: E, properties: {}): E =>
Object.freeze({
...e,
properties: Object.assign({}, e.properties, properties),
properties: _.merge({}, e.properties, properties),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christianvogt FYI, this change affects translation of strings within extension's properties in useExtensions hook:

mergeExtensionProperties(e, translate(e.properties, t))

There should be no change in above behavior, since the translate function returns the "full" object (except for strings which may get replaced and container arrays & objects having the same content but different referential identity).

Copy link
Member

@jerolimov jerolimov Aug 2, 2021

Choose a reason for hiding this comment

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

@vojtechszocs This is the change which breaks the guided tour! See other comment.

@@ -8,7 +8,8 @@
"experimentalDecorators": true,
"sourceMap": true,
"noUnusedLocals": true,
"skipLibCheck": true
"skipLibCheck": true,
"lib": ["es2020", "dom"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rawagner @christianvogt FYI, this is a workaround to allow using ES2020 Promise.allSettled types consistently in different modules within the dynamic SDK package.

Based on discussion with @tetchel I think it's best to use "target": "es2020" - I'll post a separate PR for that.

According to Console README

We support the latest versions of the following browsers:

- Edge
- Chrome 
- Safari
- Firefox

IE 11 and earlier is not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative is to use "target": "es2016" with "lib": ["es2016", "es2020.promise", "dom"] to add specific types we need on top of the existing target spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another alternative is to use "target": "es2016" with "lib": ["es2016", "es2020.promise", "dom"] to add specific types we need on top of the existing target spec.

I've updated the PR to use this approach.

@yapei
Copy link
Contributor

yapei commented Jun 2, 2021

Tested with following steps

in one terminal,
$ git clone git@github.com:vojtechszocs/console.git
$ cd console; git checkout nested-coderefs
$ git apply nested-coderefs-plugin-test.diff
$ cd frontend; yarn install
$ cd dynamic-demo-plugin; yarn install; yarn build; yarn http-server

in another terminal,
$ cd /back/to/console/root; ./build.sh
$ ./bin/bridge -plugins console-demo-plugin=http://localhost:9001/

then check browser console logs, we can see

Dynamic plugins: [console-demo-plugin]
Loading plugin manifest from http://localhost:9000/api/plugins/console-demo-plugin/plugin-manifest.json
Loading entry script for plugin console-demo-plugin@0.0.0 from http://localhost:9000/api/plugins/console-demo-plugin/plugin-entry.js
Added plugin console-demo-plugin@0.0.0
main-chunk-7d14ad1bc734ef26df43.min.js:1 Plugin console-demo-plugin@0.0.0 is now enabled
bar.ts:4 testHandler called (e,t)=>{f.b.dispatch(y(e,t))}
bar.ts:8 testing 1
bar.ts:8 testing 2

Both testHandler and testFunc are called and executed

@yapei
Copy link
Contributor

yapei commented Jun 2, 2021

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jun 2, 2021
Copy link
Contributor

@bipuladh bipuladh left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2021
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

17 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@nemesis09
Copy link
Contributor

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2021
@jerolimov
Copy link
Member

jerolimov commented Jul 28, 2021

Hey @vojtechszocs, I investigate why the E2E tests fails. They missed the guided tour when the user switches the first time to the developer perspective.

And thanks these tests I can confirm with some manual tests now that this alert / modal doesn't appear anymore. The guided tour is also not shown anymore in the dropdown menu in the top right corner.

The component is included here https://github.com/openshift/console/blob/master/frontend/public/components/masthead-toolbar.jsx#L361-L363

Debugging shows that the tour in GuidedTourMastheadTrigger is not defined. The Provider itself is defined here https://github.com/openshift/console/blob/master/frontend/packages/console-app/console-extensions.json#L24-L30

The useTourValuesForContext in https://github.com/openshift/console/blob/master/frontend/packages/console-app/src/components/tour/tour-context.ts#L122-L171 returns a tour and loaded=true. Anywhere these data are got lost?

Edit: Looks like it is related to this change: #9070 (review)

Menu for kubeadmin before (it is similar for devs):

before-kubeadmin-menu

Menu for kubeadmin with this change (it is similar for devs):

after-kubeadmin-menu

@jerolimov
Copy link
Member

jerolimov commented Aug 3, 2021

The last comment above shows that this PR breaks the devconsole Guided Tour. As mentioned, the change from Object.assign to _.merge in store.ts breaks this. See #9070 (review)

/lgtm cancel

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2021
@vojtechszocs
Copy link
Contributor Author

@jerolimov Thanks so much for looking into this! I'll rebase & update PR accordingly.

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2021
import { Extension } from '../typings';

export const isTranslatableString = (value): value is string => {
return typeof value === 'string' && /^%.+%$/.test(value);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like micro optimizations, but I also think its not necessary to create/compile/match a regex pattern here.

Suggested change
return typeof value === 'string' && /^%.+%$/.test(value);
return typeof value === 'string' && value.startsWith('%') && value.endsWith('%');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we can use startsWith '%' and endsWith '%' and length > 2 condition instead of RegExp here.

@jerolimov
Copy link
Member

Thanks for the update. The Guided tour is back again. I also tested a code ref on a second level by changing the ContextProvider extension locally. Works fine. 👍

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2021
@jerolimov
Copy link
Member

✖ addFlow/create-from-devfile.feature
✖ addFlow/create-from-docker-file.feature
✖ addFlow/create-from-git.feature

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2021
@vojtechszocs
Copy link
Contributor Author

Rebased, updated isTranslatableString function & added a unit test.

@jerolimov
Copy link
Member

/test e2e-gcp-console

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

Tested nested code refs again and works. Guided tour also works.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, christianvogt, jerolimov, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot merged commit 1a40971 into openshift:master Aug 10, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/sdk Related to console-plugin-sdk component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants