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

[PFX-850] - Fix server middleware stack discrepancies #1011

Merged
merged 4 commits into from
Jan 21, 2025
Merged

Conversation

mmason2-godaddy
Copy link
Contributor

Summary

  • Update API app generated files to use a app-level plugin for route definitions
  • Remove the use of the server framework actions - getExpressApp and getFastifyApp
  • Tune generated server files
  • Deprecate server framework actions to retrieve app instance

An issue was discovered around the implementation/usage of the getExpressApp and getFastifyApp actions. With the updated patterns of v7, there is an opportunity for the app references to have different values at different points in time. By allowing Gasket users to define server routes in multiple places, we then allow for the app reference to contain different values depending on when and where it's referenced.

In Gasket v7 we import the ./routes/index.js into the server.js and the route definitions would use an action from one of the plugins.

// ./routes/index.js
import gasket from '../gasket.js';

const app  = gasket.actions.getExpressApp();

app.use(mycoolmiddleware)

app.get('/route-uses-coolmiddleware', (req, res, next) => {
  console.log('coolmiddleware');
  next();
});
// server.js
import gasket from '../gasket.js';
import './routes/index.js` // routes it invoked first

const app = gasket.actions.getExpressApp();

app.use(waycoolmiddleware);

app.get('/routes-uses-waycoolermiddleware', (req, res, next) => {
  console.log('waycoolermiddleware');
  next();
});

gasket.actions.startServer();
// ./plugins/my-middleware.js
export default {
  name: 'my-middleware',
  hooks: {
    middleware(gasket) {
      return (req, res, next) => {
        console.log('my-middleware');
        next();
      }
    }
}

The result:

GET /route-uses-coolmiddleware // logs 'coolmiddleware'

GET /routes-uses-waycoolermiddleware // logs 'waycoolermiddleware'

// Neither route will have the middleware from the lifecycle applied

Reason

The app routes and middleware are applied at different points during the execution of the application. The routes defined in the ./routes/index.js are declared with the app after the prepare hook(essentially the routes will only have scope/access to the middleware defined in that specific file). The server.js has the same issue, routes declared will only have access to middleware declared in the server.js. We then reach the createServers -> express -> middleware hooks and by this time the routes in server.js and ./routes/index.js have already been defined with an app reference that did not contain the plugin middleware results in the middleware stack.

Solution

We are opting to use Gasket functionality to solve this problem. API apps will be generated with a app-level plugin called the routes-plugin. The local plugin will hook the server specific lifecycle(express or fastify) and set routes appropriately. The actions associated with retrieving the app reference have also been deprecated and their use with the createServers hooks has been removed. Documentation has also been updated and the metadata entries have been deprecated.

Changelog

  • Fix server middleware stack discrepancies

Test Plan

Tests updated

…re-apply old pattern, update generated files to create app-level routes plugin
…re-apply old pattern, update generated files to create app-level routes plugin
@mmason2-godaddy mmason2-godaddy requested a review from a team as a code owner January 15, 2025 22:21
@mmason2-godaddy mmason2-godaddy merged commit 2a035fc into main Jan 21, 2025
2 checks passed
@mmason2-godaddy mmason2-godaddy deleted the PFX-850 branch January 21, 2025 17:21
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.

3 participants