[FIX JENKINS-35860] - extension point component type filter #308

kzantow · 2016-06-30T15:02:01Z

Related to issue # 35860 .

Summary of this pull request:
. Add component type filter for extensionPoints

@reviewbybees

ghost · 2016-06-30T15:07:34Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

…nsion-point-types

tfennelly · 2016-07-01T11:38:47Z

js-extensions/EXTENSIONS.md

- `getExtensions(extensionPointName, [type,] onload)`
-    This method will async load data and type information as needed, and call the onload handler with a list of extension exports, e.g. the React classes or otherwise exported references.
+- `getExtensions(extensionPointName, [filter,] onload)`
+    This method will async load data and filter the information as needed, and call the onload handler with a list of extension exports, e.g. the React classes or otherwise exported references.


I'm imagining what this might mean here, but I think a concrete use case example would help. "filter as needed" is a bit ambiguous.

If the filtering is specific to the component dataType field, then I would prefer that to be explicit in this API i.e. getExtensions(extensionPointName, [dataTypes,] onload).

tfennelly · 2016-07-01T12:03:16Z

@kzantow Any particular reason why the contents of EXTENSIONS.md are not in README.md ?

michaelneale · 2016-07-04T09:52:45Z

I like it. As @tfennelly mentioned, perhaps some docs on what componentType vs dataType is (componentType is a react component name, right?)

…nsion-point-types

* EXTENSIONS -> README * more documentation for dataType vs. componentType * modify usage of 'type' to 'dataType' for consistency

kzantow · 2016-07-06T01:13:48Z

@michaelneale componentType is a react component type or other javascript type (if it's not a React extension point). I've labeled it this way to be consistent with the component term in the yaml file.

michaelneale · 2016-07-06T04:21:25Z

Looking good. So my take is that dataType is the type provided to as data to the extension, and componentType is the type helps filter things down if many things offer to implement that extension point?

@tfennelly is this clear enough now? I'm not sure if I have it right yet.

michaelneale · 2016-07-06T04:27:59Z

OK I think 🐝 from me. @tfennelly speak now or ...

I am slightly confused that different componentTypes types could provide the same named extensionPoint, but I am sure there is a good reason. (I can contrive situations, but I assume there was a real need for this).

tfennelly · 2016-07-06T08:30:29Z

js-extensions/README.md

+    This will return a list of type information, from the [classes API](../blueocean-rest/README.md#classes_API), this method also handles caching results locally.
+
+- `init()`
+    Required to be called with `{ extensionDataProvider: ..., typeInfoProvider: }` see: [ExtensionStore.js](src/ExtensionStore.js#init) for details. This is currently done in [init.jsx](../blueocean-web/src/main/js/init.jsx), with methods to fetch extension data from `<jenkins-url>/blue/js-extensions/` and type information from `<jenkins-url>/blue/rest/classes/<class-name>`.


I think it should be made more explicit (it is somewhat implicit in "This is currently done in ...", but not sure that helps a lot) that this method is not something that users of this API should ever call (? in fact, is it idempotent ?). I'd nearly not publicise it's existence at all.

Fair enough, I'll move this to some docs in the code, I guess..

tfennelly · 2016-07-06T10:39:38Z

I dunno ... I think I do get the theoretical difference between dataType and componentType, but I don't really like the impl from a usability perspective because I think it's going to be a bit confusing for people. It's just a bit too subtle. Perhaps that can be fix by more docs, but it just smells like this is not right to me and there might be a way of doing this that has less potential for confusing people.

Have we hit a concrete use case for needing both of these yet? If we have, can we articulate that somewhere and then eventually in the docs. A clear real-world example always helps.

kzantow · 2016-07-06T12:45:55Z

@tfennelly "Have we hit a concrete use case for needing both of these yet? If we have, can we articulate that somewhere and then eventually in the docs. A clear real-world example always helps."

No, we don't really have a concrete use-case.

dataType we have concrete use cases and it's modeled after Jenkins' current extensibility mechanism, with the extensionPoint essentially being the view to display. So, <ExtensionClass>/<view> is analogous to <dataType>/<extensionPoint>. In Java, this ensures the data is of a certain type and only returns views pertinent to the specific data available, but enforces nothing about the view.

The componentType enforces the returned view component matches an expected type - e.g. a list of links might require a Link type.

I'm all ears how to accomplish this differently/better but I'm not sure there's an easier way from the end-user perspective... the issue is this is bridging the gap between Java classes and JavaScript components, there's nothing else that has any sort of this metadata available AFAIK.

@michaelneale a basic case for multiple views/extensionPoints for a dataType, I think is: it's possible you have a link on a page to go to a special details view when some type of data is present, so you have the page actions like Jenkins has today, which may be extensionPoint: jenkins.thing.actions and you click a link which might render extensionPoint: jenkins.thing.view. But neither the link nor the details view would be shown for this particular plugin if it's a different data type.

kzantow · 2016-07-06T13:16:22Z

@imeredith @michaelneale - NPM fetch is broken on the PR builder, ideas?

michaelneale · 2016-07-07T02:32:45Z

@kzantow yeah failure was due to npm infra, It was passing before.
Currently having other (unrelated issues)

michaelneale · 2016-07-07T02:34:28Z

I am ok with this 🐝 - @tfennelly is this go or no go? @cliffmeyers ?

…nsion-point-types

kzantow · 2016-07-07T02:48:22Z

@michaelneale i'm 🐛 'ing this, talked with @tfennelly and we have a slightly different approach, should have it wrapped up today

michaelneale · 2016-07-07T02:53:45Z

@kzantow ok, good to know.

…nsion-point-types

sophistifunk · 2016-07-12T04:04:59Z

js-extensions/src/ExtensionRenderer.jsx

@@ -1,6 +1,6 @@
 var React = require('react');
 var ReactDOM = require('react-dom');
-var ExtensionStore = require('./ExtensionStore.js');
+var ExtensionStore = require('./ExtensionStore.js').instance;


How come we have a .instance property, rather than just exporting a single object, or a .getInstance() helper if we have a singleton but need some future-proofing?

@sophistifunk I'm all ears for a better way to do this, I would love to have some sort of DI to be able to hook this together rather than relying on init() method calls

I'm looking at this instance export and I don't really see the need for it Vs just using statics (same on ClassMetadataStore) e.g. ExtensionStore.getExtensions(...) or ClassMetadataStore.componentType( ..)?

I actually see how in the tests you have ...

var ClassMetadataStore = require('../dist/ClassMetadataStore').instance;

And then make the usage look as though it's a static e.g. ClassMetadataStore.componentType(...).

Well, I am morally opposed to statics in almost all cases. I need to have a particular instance configured by the user (e.g. blueocean-web), though, so we don't have another mechanism to do this. I think we should investigate a DI system. The tight coupling of some things has me already feeling worried about the future of this codebase...

sophistifunk · 2016-07-12T04:07:02Z

LGTM

michaelneale · 2016-07-12T05:32:25Z

🐝

michaelneale · 2016-07-12T23:00:49Z

This is pending review from @tfennelly

tfennelly · 2016-07-14T14:13:34Z

js-extensions/src/ClassMetadataStore.js

+            extensions = extensions.filter(m => !('dataType' in m));
+            onload(extensions);
+        };
+    }


Do componentType, dataType and untyped have anything to do with ClassMetadataStore ? Seems like they belong more on ExtensionStore, seeing as though they are for filtering the extensions in their.

Is there scope here to formalise the Filter mechanism into a type and extend it to create ComponentTypeFilter, DataTypeFilter etc in class files of their own?

tfennelly · 2016-07-14T14:19:46Z

🐝

Though I don't really like the instance approach on ExtensionStore and ClassMetadataStore + some of the code seems to be in the wrong'ish place imo + I'd like to see the Filter mechanism formalised a bit more Vs bolting it onto the side of ClassMetadataStore (which seems like the wrong place to me anyway).

kzantow · 2016-07-14T14:36:40Z

@tfennelly the filter is about as 'formalized' as it should be, I think? The specific filters are dependent on the type information, therefore located with the class that provides it (well, except the componentType, which should be moved, probably)

…nsion-point-types

tfennelly · 2016-07-15T14:14:23Z

@kzantow okioki ... I'd prob try formalize it more, but maybe that's just personal taste.

🐝 good to go !!

…nsion-point-types

kzantow added 3 commits June 30, 2016 15:01

Add component type filter for extensionPoints

7a4cc9c

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

8d3c0e0

…nsion-point-types

Bump js-extensions beta version

f41a636

tfennelly reviewed Jul 1, 2016
View reviewed changes

tfennelly added the needs-review label Jul 1, 2016

kzantow added 2 commits July 5, 2016 09:13

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

4ebf1cf

…nsion-point-types

Address @tfennelly's comments:

c24dfaf

* EXTENSIONS -> README * more documentation for dataType vs. componentType * modify usage of 'type' to 'dataType' for consistency

tfennelly reviewed Jul 6, 2016
View reviewed changes

kzantow closed this Jul 6, 2016

kzantow reopened this Jul 6, 2016

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

7e21eda

…nsion-point-types

michaelneale added work-in-progress and removed needs-review labels Jul 7, 2016

kzantow added 2 commits July 11, 2016 11:02

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

d09af3e

…nsion-point-types

Split ClassMetadataStore, modify extension filtering to a common method

8983233

kzantow added the needs-review label Jul 11, 2016

Burned through some npm versions to get this published with a tag, d'oh

3397771

sophistifunk reviewed Jul 12, 2016
View reviewed changes

michaelneale added enhancement help wanted work-in-progress and removed needs-review labels Jul 12, 2016

tfennelly reviewed Jul 14, 2016
View reviewed changes

kzantow added 2 commits July 15, 2016 22:42

Separate componentType to its own file

fdbe5a6

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

479bc8e

…nsion-point-types

kzantow added needs-review and removed enhancement help wanted work-in-progress labels Jul 15, 2016

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

2a808b2

…nsion-point-types

michaelneale added work-in-progress and removed needs-review labels Jul 19, 2016

Merge remote-tracking branch 'primary/master' into JENKINS-35860-exte…

0dd3d55

…nsion-point-types

kzantow added ready-for-merge and removed work-in-progress labels Jul 20, 2016

kzantow mentioned this pull request Jul 20, 2016

[WiP] First class data types / extra extension points / extensible router #362

Closed

6 tasks

Bump js-extensions version to non-beta

66804ef

kzantow merged commit 7eaf71b into jenkinsci:master Jul 20, 2016

kzantow deleted the JENKINS-35860-extension-point-types branch July 20, 2016 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX JENKINS-35860] - extension point component type filter #308

[FIX JENKINS-35860] - extension point component type filter #308

kzantow commented Jun 30, 2016

ghost commented Jun 30, 2016

tfennelly Jul 1, 2016

tfennelly Jul 1, 2016

tfennelly commented Jul 1, 2016

michaelneale commented Jul 4, 2016

kzantow commented Jul 6, 2016

michaelneale commented Jul 6, 2016

michaelneale commented Jul 6, 2016

tfennelly Jul 6, 2016

kzantow Jul 6, 2016

tfennelly commented Jul 6, 2016

kzantow commented Jul 6, 2016 •

edited

Loading

kzantow commented Jul 6, 2016

michaelneale commented Jul 7, 2016

michaelneale commented Jul 7, 2016

kzantow commented Jul 7, 2016

michaelneale commented Jul 7, 2016

sophistifunk Jul 12, 2016

kzantow Jul 12, 2016

tfennelly Jul 14, 2016

kzantow Jul 14, 2016

sophistifunk commented Jul 12, 2016

michaelneale commented Jul 12, 2016

michaelneale commented Jul 12, 2016

tfennelly Jul 14, 2016

tfennelly commented Jul 14, 2016

kzantow commented Jul 14, 2016

tfennelly commented Jul 15, 2016

[FIX JENKINS-35860] - extension point component type filter #308

[FIX JENKINS-35860] - extension point component type filter #308

Conversation

kzantow commented Jun 30, 2016

ghost commented Jun 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfennelly commented Jul 1, 2016

michaelneale commented Jul 4, 2016

kzantow commented Jul 6, 2016

michaelneale commented Jul 6, 2016

michaelneale commented Jul 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfennelly commented Jul 6, 2016

kzantow commented Jul 6, 2016 • edited Loading

kzantow commented Jul 6, 2016

michaelneale commented Jul 7, 2016

michaelneale commented Jul 7, 2016

kzantow commented Jul 7, 2016

michaelneale commented Jul 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophistifunk commented Jul 12, 2016

michaelneale commented Jul 12, 2016

michaelneale commented Jul 12, 2016

Choose a reason for hiding this comment

tfennelly commented Jul 14, 2016

kzantow commented Jul 14, 2016

tfennelly commented Jul 15, 2016

kzantow commented Jul 6, 2016 •

edited

Loading