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(core): add path property to modules #297

Closed
wants to merge 5 commits into from
Closed

feat(core): add path property to modules #297

wants to merge 5 commits into from

Conversation

wbhob
Copy link
Contributor

@wbhob wbhob commented Dec 9, 2017

Some apps may quickly become very large, and the current nest architecture does not allow much room
to scale. This commit adds a path property on module metadata. This way, one can maintain good
modular structure without worrying about maintaining the right nested paths. It takes advantage of
the path package in Node.JS to concatenate paths without worrying about slashes (this may need to be
changed or standardized in the future, by checking for slashes in the first or last position of the
string). This is a MINOR VERSION change, and should not be merged until the next minor is about to
release.

Fixes #255

Some apps may quickly become very large, and the current nest architecture does not allow much room
to scale. This commit adds a `path` property on module metadata. This way, one can maintain good
modular structure without worrying about maintaining the right nested paths. It takes advantage of
the path package in Node.JS to concatenate paths without worrying about slashes (this may need to be
changed or standardized in the future, by checking for slashes in the first or last position of the
string). This is a MINOR VERSION change, and should not be merged until the next minor is about to
release.

Fixes #255
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 94.661% when pulling bd026d4 on wbhob:module-prefix into 853c3c9 on nestjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 94.69% when pulling 38dd56b on wbhob:module-prefix into 853c3c9 on nestjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0001%) to 94.754% when pulling 8626f9c on wbhob:module-prefix into 853c3c9 on nestjs:master.

@wbhob
Copy link
Contributor Author

wbhob commented Dec 12, 2017

@coveralls you are mean because nobody cares about that ten-thousandth of a percent. Nobody.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0002%) to 94.708% when pulling b9e3ebd on wbhob:module-prefix into 4d4f7a4 on nestjs:master.

@shekohex
Copy link
Contributor

You made it 😀 🎉

@wbhob
Copy link
Contributor Author

wbhob commented Dec 15, 2017

Hahaha I did... now just to convince Kamil to merge 😄

@shekohex
Copy link
Contributor

@coveralls are you satisfied now ?
You have +0.0002%

@kamilmysliwiec
Copy link
Member

Hi @wbhob,
I explained why it's not a good idea to put the path property into the @Module() decorator. Please, remember that modules may create a graph as well, not only the tree structure. Then this solution may bring side effects. Also, right now it's possible to use Nest as a: web app, microservice, and as a server side app (for CRON's for example) without transport layer. Modules are reused across all of them, and the path property is used only for the single use-case. That's not good 🙂

@wbhob
Copy link
Contributor Author

wbhob commented Dec 21, 2017

If there’s only one use case, people that don’t need it could just not use it...

@wbhob
Copy link
Contributor Author

wbhob commented Dec 21, 2017

However there needs to be some solution. It’s not DRY to paste 5- or 6-level deep URLs into each controller.

@kamilmysliwiec
Copy link
Member

RouterModule. It should be really easy to build something like here #255. I just have much more packages on my mind right now

@shekohex
Copy link
Contributor

shekohex commented Dec 21, 2017

Actually , I think I made something like RouterModule , take a look here but It was depend on having access to Nest Container , but I will Refactor it to make it stand-alone.

@kamilmysliwiec
Copy link
Member

@shekohex I need to update the docs with ModulesContainer. You should be able to do that inside the Nest app then

@kamilmysliwiec
Copy link
Member

For now you can take a look into nestjs/graphql module

@shekohex
Copy link
Contributor

ModulesContainer and extractMetadata, it seems good for me, I will have a look at it tonight 👍

@wbhob
Copy link
Contributor Author

wbhob commented Dec 21, 2017

Yeah, if that functionality exists that may even be a lot better than my implementation. Gotta think about enterprise!

@wbhob
Copy link
Contributor Author

wbhob commented Dec 23, 2017

You’ll also notice my implementation is not dependent on having a path set at all, and it only applies to controllers.

@lock
Copy link

lock bot commented Sep 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global prefix for a module / route tree
4 participants