feat(rest): allow basePath for rest servers

[raymondfeng]

See #918

Some deviation from #918:

• The basePath is only available as part of RestServerConfig because we set up express routes within the constructor.
• The basePath only applies to registered routes (including static assets)
• servers in openapi spec are adjusted accordingly

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[bajtos]

• Have you considered other approaches for the basePath configuration option?
• How is config.basePath going to interact with the values provided in config.openApi.servers?
• How will config.basePath affect paths provided via app.api(), @api() decorator at controller level and @operation decorators at controller-method level?

[bajtos]

The basePath is only available as part of RestServerConfig because we set up express routes within the constructor.

Can this limitation affect usability of the feature when configuring basePath from an application?

Please add an acceptance test showing how a RestApplication can configure basePath. Try to mimic the coding conventions we are using in our example applications (e.g. examples/todo/src/application.ts) - a custom class extending RestApplication where the configuration options are modified inside application's constructor.

[bajtos]

The basePath is only available as part of RestServerConfig because we set up express routes within the constructor.

Also: have you considered to refactor RestServer so that setup of express routes is deferred until we need to access the request-handler function?

[raymondfeng]

Can this limitation affect usability of the feature when configuring basePath from an application?

I don't think so. IMO, basePath is in the same category as host and port for REST servers and we don't have APIs to set up such values beyond configuration.

Anyway, I refactored rest-server.ts a bit so that the Express app will only be set up if needed. I also added basePath() to RestApplication and RestServer.

PTAL.

[bajtos]

Documentation in /docs/site was updated

Would you mind adding a short documentation for basePath?

Affected artifact templates in packages/cli were updated

Should we extend Application template to explicitly call this.basePath so that users know where to start when changing the base path?

Affected example projects in examples/* were updated

Is there anything to update here?

[bajtos]

Looks mostly good, please address my two comments below.

No further review is required from my side.

[bajtos]

I find this test very confusing. As far as I understand givenApplication() helper, it creates an app with no API routes configured. As I understand app.basePath, it's intended primarily for customizing the base path of REST APIs.

I see that there are other tests verifying the impact of basePath on different kinds of routes, I agree there is no need to duplicate those tests in rest.application.integration.ts.

Can you please add a comment here? Explain that in this test, we are invoking a route for static assets, because basePath applies to all kinds of routes, including static assets.

Alternatively, rework this test to use a handler-function route (see the example below) - I think it will make the intent of this test much easier to understand even without comments.

app.route('get', '/status', {/*spec*/}, () => {running: true});
// ...
client.get('/api/status').expect(200, {running: true});

[bajtos]

If this condition happens to be false, then server.basePath is not invoked at all and the test fails with a confusing error.

Please convert this if into an explicit assertion. For example:

expect.assert(!!server.requestHandler, 'requestHandler should have been set up by now');
expect(() => server.basePath('/api')).to.throw(/*...*/);

[AnanthGopal]

hi @raymondfeng

Thanks to every loopback developer. when we will use this "Add basePath configuration" feature ?