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

feat(config): add extraRoutes to scully.config #139

Merged
merged 2 commits into from
Jan 4, 2020

Conversation

aaronfrost
Copy link
Contributor

With the additional of extraRoutes to the scully.config, users can now provide additional routes
that they want Scully to discover. These routes can plug into the router plugins as well, and can
produce multitudes of browsable routes each.

I need this feature to integrate with our first big enterprise client.

In your scully config, you can now provide an extraRoutes that looks like an Angular route (meaning it contains parameters). These can be routes that exist in AngularJS, or in React, or in whatever Framework's router. What's really important is that this unlocks our ability to support Angular Hybrid scenarios where some routes are controlled by Angular and others controlled by AngularJS. The Guessjs parser will find the Angular routes. But won't find the AngularJS routes. This extraRoutes field allows me a place to specify all of the AngularJS routes I would like Scully to render as well.

exports.config = {
    ...
    extraRoutes: [
        "/donuts",
        "/donuts/:donutId"
    ]
}

The example would now have a route with a parameter in it. I could then configure a routerPlugin to fetch those donut id. I would do that the same as if I was doing it for an Angular route.

exports.config = {
    ...
    routes: {
        "/donuts/:donutId": {
            type: "myCustomDonutIdPlugin"
        }
    },
    extraRoutes: [
        "/donuts",
        "/donuts/:donutId"
    ]
}

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

With the additional of extraRoutes to the scully.config, users can now provide additional routes
that they want Scully to discover. These routes can plug into the router plugins as well, and can
produce multitudes of browseable routes each.
@@ -62,3 +65,16 @@ async function doChunks(dataRoutes) {
return x.concat(activeChunk);
}, Promise.resolve([]));
}

function addExtraRoutes(appRoutes, config){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of simply allowing people to provide a string[] as the type of config.extraRotues, we could allow the type of extra routes to be (string | string[] | Promise | Promise<string[]>)[], which we could just flatten it all and then turn them all into promises, and run a Promises.all([flattenedAndPromisifiedExtraRoutes]) and then return them.

That is likely the most extensible and support the most amount of people. But... for now, I support this list of strings gets us quite a ways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have the promise<string[]>, as that is indeed the most extensible.
For now, I can live with this. I probably need to do a bit of redesign anyway, so we can start generating "pages" before we are done collecting all routes anyway.

Copy link
Contributor

@SanderElias SanderElias left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -62,3 +65,16 @@ async function doChunks(dataRoutes) {
return x.concat(activeChunk);
}, Promise.resolve([]));
}

function addExtraRoutes(appRoutes, config){
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to have the promise<string[]>, as that is indeed the most extensible.
For now, I can live with this. I probably need to do a bit of redesign anyway, so we can start generating "pages" before we are done collecting all routes anyway.

I converted this process to return a `Promise<unhandledRoutes[]>` which is what we needed it to be
in order to allow for async fetching of extraRoutes at build time. This is actually a very wonderful
and robust method.
)
);
return x.concat(activeChunk);
}, Promise.resolve([]));
}

async function addExtraRoutes(config): Promise<string[]> {
let extraRoutes = config.extraRoutes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SanderElias please read this now. I changed it to allow you to pass a hodge-podge array of string and promises, and it will take care of resolving everything, removing empty values, and flattening nested arrays of unhandled routes.

[ '/user/:id', Promise<string>, Promise<string[]>, undefined]

gets turned into

['/user/:id', '/foo/:id', '/bar/:id', '/baz/:id']

Copy link
Contributor

Choose a reason for hiding this comment

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

awsome, that is even more as I would have wanted ;)

@SanderElias SanderElias merged commit 28dab59 into master Jan 4, 2020
@SanderElias SanderElias deleted the frosty/extraRoutes branch January 4, 2020 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants