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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: simplify middleware-based sequence #6824

Merged

Conversation

mrmodise
Copy link
Contributor

Closes #6770

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • 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

馃憠 Check out how to submit a PR 馃憟

@mrmodise

This comment has been minimized.

@mrmodise mrmodise force-pushed the simplifying-middleware-based-sequence branch 2 times, most recently from 07f7650 to 43a01a6 Compare November 23, 2020 04:20
@mrmodise mrmodise marked this pull request as ready for review November 23, 2020 04:20
@mrmodise

This comment has been minimized.

@raymondfeng
Copy link
Contributor

@mrmodise Thank you for the PR.

For sequences, we basically have two styles:

  1. The legacy action based sequence - it consists of a series of actions in form of functions that are pre-generated in the src/sequence.ts. LoopBack ships DefaultSequence as the base.

  2. The new middleware-based sequence - it is a middleware chain that discovers and invokes middleware. The base class is MiddlewareSequence.

Both sequence are extensible and customizable. The legacy sequence has a middleware action that allows plugging in additional middleware. The new sequence is fully composable from middleware via the extension point/extension pattern. Each style has a base class to be extended for customization.

@mrmodise
Copy link
Contributor Author

Thanks @raymondfeng based on your comments I think even the action-based sequence needs some improvement since the section is mixing a lot of complex topics. Let me do some optimisations there as well. You can review the current changes on middleware-based since thats the one I had targeted

@mrmodise mrmodise marked this pull request as draft November 23, 2020 06:13
@mrmodise mrmodise force-pushed the simplifying-middleware-based-sequence branch from 43a01a6 to d2b831a Compare November 23, 2020 06:46
@mrmodise mrmodise marked this pull request as ready for review November 23, 2020 06:47
@mrmodise mrmodise force-pushed the simplifying-middleware-based-sequence branch 2 times, most recently from 738dae1 to 4d1ad7e Compare November 23, 2020 08:31
@mrmodise mrmodise requested a review from bajtos November 27, 2020 04:04
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

@mrmodise, thanks for the PR. I had some minor comments.
For the files to be deleted, I'm wondering if there's an equivalent file for that. If yes, it would be helpful to add redirect_from in the front matter of the md file (example: https://raw.githubusercontent.com/strongloop/loopback-next/master/docs/site/Accessing-services.md). Thanks.

docs/site/Advanced-middleware-based-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-middleware-based-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-middleware-based-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-sequence-topics.md Show resolved Hide resolved
@mrmodise
Copy link
Contributor Author

https://raw.githubusercontent.com/strongloop/loopback-next/master/docs/site/Accessing-services.md

@mrmodise, thanks for the PR. I had some minor comments.
For the files to be deleted, I'm wondering if there's an equivalent file for that. If yes, it would be helpful to add redirect_from in the front matter of the md file (example: https://raw.githubusercontent.com/strongloop/loopback-next/master/docs/site/Accessing-services.md). Thanks.

I will double check this

@mrmodise
Copy link
Contributor Author

mrmodise commented Dec 29, 2020

Discovered gaps while working on 977 cc @achrinza

@mrmodise mrmodise force-pushed the simplifying-middleware-based-sequence branch 2 times, most recently from c134140 to 3ebe64c Compare January 28, 2021 03:51
@mrmodise mrmodise force-pushed the simplifying-middleware-based-sequence branch 2 times, most recently from 307d543 to 9e936d1 Compare February 21, 2021 06:28
@mrmodise mrmodise marked this pull request as ready for review February 21, 2021 06:29
Copy link
Member

@achrinza achrinza left a comment

Choose a reason for hiding this comment

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

Great work thus far! 馃憤

Sorry I don't have time to fully look through the changes in detail. But this is my initial comments 馃憞

docs/site/Advanced-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-sequence-topics.md Outdated Show resolved Hide resolved
docs/site/Advanced-sequence-topics.md Outdated Show resolved Hide resolved
Signed-off-by: mrmodise <modisemorebodi@gmail.com>
@mrmodise mrmodise force-pushed the simplifying-middleware-based-sequence branch from 9e936d1 to 6ab15cc Compare February 23, 2021 04:46
@mrmodise
Copy link
Contributor Author

@mrmodise, thanks for the PR. I had some minor comments.
For the files to be deleted, I'm wondering if there's an equivalent file for that. If yes, it would be helpful to add redirect_from in the front matter of the md file (example: https://raw.githubusercontent.com/strongloop/loopback-next/master/docs/site/Accessing-services.md). Thanks.

For the pages to be deleted i split the content across other pages

PTAL @dhmlau

@bajtos
Copy link
Member

bajtos commented Feb 23, 2021

@mrmodise, thanks for the PR. I had some minor comments.
For the files to be deleted, I'm wondering if there's an equivalent file for that. If yes, it would be helpful to add redirect_from in the front matter of the md file (example: https://raw.githubusercontent.com/strongloop/loopback-next/master/docs/site/Accessing-services.md). Thanks.

For the pages to be deleted i split the content across other pages

I reviewed the new version, no pages are being removed now. Existing links pointing to our docs should keep working after this change.

@bajtos bajtos merged commit d594119 into loopbackio:master Feb 23, 2021
@bajtos
Copy link
Member

bajtos commented Feb 23, 2021

Landed, thank you @mrmodise for the contribution! 鉂わ笍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: simplify middleware based sequence
6 participants