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

Pass options to middleware #9205

Merged
merged 1 commit into from May 12, 2020
Merged

Conversation

mrloop
Copy link
Contributor

@mrloop mrloop commented May 1, 2020

Useful if the middleware is expensive and should be optionally started
depending on ember-cli options.

For example ember serve and ember test use a path flag to specify
an existing ember build to use, without rebuilding. ember-cli-typescript
will add typechecking middleware regardless of ember using an existing
build, the typechecking middleware is not required. This can slow
down the initial request to express server significantly. Passing the
ember options to middleware will allow middleware to optionally enable
different features depending on options

See ember-cli-typescript making use of this here typed-ember/ember-cli-typescript#1148

@mrloop
Copy link
Contributor Author

mrloop commented May 1, 2020

A couple of questions:

  • not sure what test would be appropriate, check addonMiddlewares receives options in tests/unit/tasks/test-test.js?
  • should the options passed to middlewares be transformed, be a subset? For example only pass {hasBuild: true} if original options contain path?

Copy link
Member

@rwjblue rwjblue left a comment

should the options passed to middlewares be transformed, be a subset? For example only pass {hasBuild: true} if original options contain path?

No, I don't think so. As mentioned in another comment the serverMiddleware hook already receives the unfettered options so this seems fine to me.

not sure what test would be appropriate, check addonMiddlewares receives options in tests/unit/tasks/test-test.js?

If possible, I'd setup a test that actually adds an addon that uses testemMiddleware and assert that it receives the options.

addons.push(function () {
return addon.testemMiddleware.apply(addon, [].slice.call(arguments).concat([options]));
});
Copy link
Member

@rwjblue rwjblue May 6, 2020

Choose a reason for hiding this comment

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

I think this will work on all of our supported Node versions:

Suggested change
addons.push(function () {
return addon.testemMiddleware.apply(addon, [].slice.call(arguments).concat([options]));
});
addons.push(function () {
return addon.testemMiddleware(...arguments, options);
});

Copy link
Contributor Author

@mrloop mrloop May 10, 2020

Choose a reason for hiding this comment

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

👍 have updated as suggested

@mrloop mrloop force-pushed the middleware-options branch 6 times, most recently from 5972bba to 880a89a Compare May 10, 2020
@mrloop
Copy link
Contributor Author

mrloop commented May 10, 2020

I'd setup a test that actually adds an addon that uses testemMiddleware and assert that it receives the options.

@rwjblue have added the suggested test. It configures a dummy test launcher for the test, so full stack is run, apart from the test apps tests.

@mrloop mrloop requested a review from rwjblue May 10, 2020
Copy link
Member

@rwjblue rwjblue left a comment

Awesome, thank you!!!

@rwjblue rwjblue changed the base branch from master to beta May 11, 2020
Useful if the middleware is expensive and should be optionally started
depending on ember-cli options.

For example `ember serve` and `ember test` use a `path` flag to specify
an existing ember build to use, without rebuilding. `ember-cli-typescript`
will add typechecking middleware regardless of ember using an existing
build, the typechecking middleware is not required. This can slow
down the initial request to express server significantly. Passing the
ember options to middleware will allow middleware to optionally enable
different features depending on options
@rwjblue
Copy link
Member

rwjblue commented May 11, 2020

I've updated the target of this PR and rebased against the beta branch. Should be ready to land once CI is green.

@rwjblue rwjblue merged commit fb7b8e9 into ember-cli:beta May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants