Multiple outlets #110

umaar · 2017-10-25T09:42:10Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide - I thought automated linting picks this up?
Unit or Functional tests are included in the PR

Description:

Enhance getOutlet so it can receive a single outlet ID or an array of outlet IDs.

Resolves #103

umaar · 2017-10-25T09:43:57Z

Can someone update the repo settings so it prevents merging without an approval? Also the travis CI stuff hasn't finished but the merge button is still active.

kitsonk · 2017-10-25T10:06:51Z

Can someone update the repo settings so it prevents merging without an approval?

We have tried that, that causes issues with our release scripts, because we push some additional commits directly to master when we do a release. Restricting it only to admins then prevents non-admins from publishing a release, which is undesirable. So we work on the honour system for those that have commit access.

Also the travis CI stuff hasn't finished but the merge button is still active.

There are some transitory states when the additional checks. Initially the merge button will appear active, but often the refresh a page, once the git hook to travis has been properly fired, will remove the check. Again, we work on the honour system for committers. Be responsible.

agubler

A couple of comments

agubler · 2017-10-26T20:44:13Z

src/Outlet.ts

@@ -30,7 +30,7 @@ export type Outlet<

 export function Outlet<W extends WidgetBaseInterface, F extends WidgetBaseInterface, E extends WidgetBaseInterface>(
 	outletComponents: Component<W> | OutletComponents<W, F, E>,
-	outlet: string,
+	outlet: string | Array<String>,


We use [] for array types, string[] - Also should avoid using String and use string instead (https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html).

Done, thanks for explaining, I will read that page. Can we lint for this sort of thing?

@umaar https://palantir.github.io/tslint/rules/ban-types/ so it appears yes... let's open an issue on https://github.com/dojo/dojo2-package-template where the master tslint.json file resides. We likely need to do a wholesale review.

Issue raised!

agubler · 2017-10-26T20:45:01Z

src/Router.ts

+	getOutlet(outletId: string | Array<String>): OutletContext | undefined {
+		if (Array.isArray(outletId)) {
+			const outletContextArray = arrayFrom(this._outletContextMap);
+			const outlets = arrayFrom(outletId);


Why do you need to arrayFrom here? We are already know it is an array as we are in the Array.isArray(outletId) block.

Ah good catch, left over code from when I was trying to convert the "needle" we are trying to find into an array, even if it was one already. Removed!

agubler · 2017-10-26T20:47:58Z

src/Router.ts

+			const outletContextArray = arrayFrom(this._outletContextMap);
+			const outlets = arrayFrom(outletId);
+
+			const matchingOutlet = find(outletContextArray, ([key]) => {


Would be nicer if you destructured this:

const [, matchingOutlet ] = find(...);

Then just return matchingOutlet

Ah yes tried that too! TypeScript was complaining about something like undefined not being an array type and I think it makes sense. The find function can return undefined, and we can't destructure undefined. Is that your understanding too?

If you are convinced that find() will not return undefined because of other checks or validations, you can use the escape hatch of the non-null assertion operator (find(...)!). Our general convention is to comment the code though why you believe you are safe using the operator.

Oh cool thanks for explaining that! In this particular case, I'm not convinced that find() will never return undefined so I think we can leave it as it is, useful to know though!

We really try and avoid using ! to force TS to ignore strictNullChecks in src code.

Could || [] after the find if it helps make the code clearer by enabling the destructuring.

I'm not sure the code becomes any clearer at that point. I tried it anyway and got:

Don't suppose you know anything about that?

agubler · 2017-10-26T20:48:18Z

src/Router.ts

+			});
+
+			return matchingOutlet ? matchingOutlet[1] : undefined;
+		} else {


Don't need the else block as we are returning from within the if block

Done. Would be really good if we linted for this, something like https://eslint.org/docs/rules/no-else-return for TS?

It doesn't appear that tslint has it as an out of the box rule (https://palantir.github.io/tslint/rules/). We haven't experimented with custom rules, but it maybe again worth opening an issue for tracking. We looked at ESLints support of TypeScript about 9 months ago, and haven't since. We know there has been work on settling on an AST syntax for TypeScript (preliminary work is already in Babel) but haven't had the time to see if ESLint has matured in that aspect. We really need something that takes the AST from the TypeScript compiler. Either way, it is likely a good idea for a separate issue to further discuss trying to move to ESLint. I can see the long term benefits TBH.

Issue raised!

agubler · 2017-10-26T20:49:22Z

src/Router.ts

@@ -439,8 +439,19 @@ export class Router<C extends Context> extends Evented implements RouterInterfac
 		return this._outletContextMap.has(outletId);
 	}

-	getOutlet(outletId: string): OutletContext | undefined {
-		return this._outletContextMap.get(outletId);
+	getOutlet(outletId: string | Array<String>): OutletContext | undefined {


string[] over Array<String> here as well.

umaar · 2017-10-27T09:42:09Z

Thanks @agubler please give another review!

agubler · 2017-10-27T14:21:23Z

@umaar what happens if multiple outlets match?

umaar · 2017-10-27T14:25:54Z

The find() method will return the first matching item. So we iterate over the 'context map' which is stored internally, and as soon as a match is found, we return it. The order in which we iterate over the context map is in the same order in which items are added to it. Do you think that's ok?

agubler · 2017-10-27T14:31:51Z

I'm not sure, because then it's dependant on the order of the outlets specified... do you think we should return all matching?

umaar · 2017-10-27T16:02:42Z

I'm not really sure to be honest! If we returned an array of all matching outlets, what would be the use/benefit of that?

agubler · 2017-10-31T10:28:07Z

@umaar I'm trying to think... It's almost as though we want the "deepest" location but the params from all matching outlets - Then these would be available in the mapParams function for the consumer to use?

If this is desirable, it would be good to have a test that includes params too (that could verify this behaviour).

So this test, would return a location of /path-2/nested-path/nested-path and the respective match type but params would be all the params from matching outlets?

routing/tests/unit/Router.ts

Lines 555 to 567 in 23f7072

    
           await router.dispatch({}, '/path-2/nested-path/nested-path'); 
        
           assert.deepEqual(router.getOutlet(['outlet-id-4']), { 
        
           	location: '/path-2/nested-path/nested-path', 
        
           	type: MatchType.INDEX, 
        
           	params: {} 
        
           }); 
        
           assert.deepEqual(router.getOutlet(['outlet-id-4', 'outlet-id-2']), { 
        
           	location: '/path-2', 
        
           	type: MatchType.PARTIAL, 
        
           	params: {} 
        
           });

umaar · 2017-11-01T11:10:15Z

Me and @agubler just had a call about this (thanks Ant very helpful 💯) and I would agree with your idea! It makes sense. Let me do that ✅

umaar · 2017-11-02T13:19:24Z

@agubler could you double check this PR (also please check if it's treating the multiple outlets in the way you expect!) thank you

agubler · 2017-11-06T13:45:39Z

src/Router.ts

-		return this._outletContextMap.get(outletId);
+	getOutlet(outletIds: string | string[]): OutletContext | undefined {
+		if (Array.isArray(outletIds)) {
+			const combinedParams = outletIds.reduce((params, cur, index) => {


This section feels quite complicated now, do you think it could be simplified slightly? We could maybe use the path to determine if a matching outlet is deeper than an existing match and not need to loop through the context map.

Rough example:

let matchingOutlet: OutletContext | undefined = undefined; let matchingParams: Parameters = {}; let matchingLocation = ''; for (let i = 0; i < outletIds.length; i++) { const outletContext = this._outletContextMap.get(outletIds[i]); if (outletContext) { const { params, location } = outletContext; matchingParams = { ...matchingParams, ...params }; if (!matchingOutlet || matchingLocation.indexOf(location) === -1) { matchingLocation = location; matchingOutlet = { ...outletContext, params: matchingParams }; } } } return matchingOutlet;

Thank you! I've just added a const outletIds = Array.isArray(outletId) ? outletId : [outletId]; to make sure we're dealing with an array, but apart from that, I'm using your example 👍

agubler · 2017-11-07T12:50:56Z

src/Router.ts

-	getOutlet(outletId: string): OutletContext | undefined {
-		return this._outletContextMap.get(outletId);
+	getOutlet(outletId: string | string[]): OutletContext | undefined {
+		const outletIds = Array.isArray(outletId) ? outletId : [outletId];


nit: code style, should have spaces [ outletId ]

agubler · 2017-11-07T12:51:41Z

@umaar could do with updating the README also?

…utlet`

umaar · 2017-11-07T14:57:05Z

@agubler readme updated

agubler

👍

umaar changed the title ~~Multiple outlet experiments~~ Multiple outles Oct 25, 2017

umaar changed the title ~~Multiple outles~~ Multiple outlets Oct 25, 2017

umaar requested review from agubler and matt-gadd October 26, 2017 16:30

agubler suggested changes Oct 26, 2017

View reviewed changes

agubler reviewed Oct 26, 2017

View reviewed changes

This was referenced Oct 27, 2017

Add the ban-types rule to tslint.json dojo/dojo-package-template#95

Open

Start using linting rules from ESLint dojo/dojo-package-template#96

Open

dylans added this to the 2017.10 milestone Oct 27, 2017

kitsonk modified the milestones: 2017.10, beta.4 Oct 30, 2017

agubler reviewed Nov 6, 2017

View reviewed changes

agubler reviewed Nov 7, 2017

View reviewed changes

umaar added 6 commits November 7, 2017 14:45

Work in progress for #103, handle an array or single string for `getO…

da2512d

…utlet`

Use an Array.from shim

92ef24c

Test for multiple matching outlets

537d5ed

Code review feedback

384ac9e

Combine params from multiple matching outlets

36b9886

Simplify get outlet method

1a3941b

Code style

4a35fd0

umaar force-pushed the multiple-outlet-experiments branch from 81d787d to 4a35fd0 Compare November 7, 2017 14:45

Add note to readme on multiple matching outlets

b7ee40e

agubler approved these changes Nov 7, 2017

View reviewed changes

umaar merged commit c3d1450 into master Nov 7, 2017

umaar deleted the multiple-outlet-experiments branch November 7, 2017 16:06

Multiple outlets #110

Multiple outlets #110

Conversation

umaar commented Oct 25, 2017

umaar commented Oct 25, 2017

kitsonk commented Oct 25, 2017

agubler left a comment

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

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

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

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

Choose a reason for hiding this comment

agubler Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umaar commented Oct 27, 2017

agubler commented Oct 27, 2017

umaar commented Oct 27, 2017

agubler commented Oct 27, 2017

umaar commented Oct 27, 2017 • edited Loading

agubler commented Oct 31, 2017

umaar commented Nov 1, 2017

umaar commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler commented Nov 7, 2017

umaar commented Nov 7, 2017

agubler left a comment

Choose a reason for hiding this comment

agubler Oct 26, 2017 •

edited

Loading

umaar commented Oct 27, 2017 •

edited

Loading