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

docs: clarify emitDecoratorMetadata requirement #1693

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camsteffen
Copy link

Description

emitDecoratorMetadata is not available in some environments so it is helpful to clarify that it is not strictly required. Also I broke out a separate sub-section for TypeScript under Installation.

Related Issue

Related: #1493

Motivation and Context

I wanted to transition to using tsx runtime which does not support emitDecoratorMetadata.

How Has This Been Tested?

Types of changes

  • Updated docs / Refactor code / Added a tests case (non-breaking change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the changelog.

emitDecoratorMetadata is not available in some environments so it is
helpful to clarify that it is not strictly required. Also I broke out a
separate sub-section for TypeScript under Installation.
@notaphplover
Copy link
Member

Hey @camsteffen, thanks for your contribution. You're absolutely right. Having said that, these docs are legacy and are about to be replaced. Would you consider having a look at the new docs? I would prefer to update the new docs in the monorepo. I think we could explain the nuances of automatic class injection in the Fundamentals section of the new docs.

Your feedback would be more than welcome in this discussion.

@camsteffen
Copy link
Author

Thanks for taking a look @notaphplover. The new docs looks promising!

I think we could explain the nuances of automatic class injection in the Fundamentals section of the new docs.

Sounds good to me. How about an Injection page that comes after the Binding page? It could cover class injection and anything else injection related.

I think it's also helpful to have an overview of the tsconfig options under Getting Started. It looks like the blurb that I edited in this PR could be very similarly applied to the new Getting Started page. I could open a PR with that change if you like.

Another thing I'd like to see in the docs is a clear explanation of what @injectable does. I noticed that it doesn't seem to be necessary in my project (kinda like emitDecoratorMetadata).

I was initially confused by docs/ vs. packages/docs/ in the monorepo. Maybe rename docs/ to contributing/.

@notaphplover
Copy link
Member

Hey @camsteffen

Sounds good to me. How about an Injection page that comes after the Binding page? It could cover class injection and anything else injection related.

Sounds great!

I think it's also helpful to have an overview of the tsconfig options under Getting Started. It looks like the blurb that I edited in this PR could be very similarly applied to the new Getting Started page. I could open a PR with that change if you like.

As long as experimentalDecorators option is enabled, any valid typescript options are perfectly fine, so I would prefer to keep it the way it is. Maybe we could add some lines in the Injection page to establish when emitDecoratorMetadata is mandatory.

Another thing I'd like to see in the docs is a clear explanation of what @Injectable does. I noticed that it doesn't seem to be necessary in my project (kinda like emitDecoratorMetadata).

Yeah, it's established in the API docs, but I honestly believe the Injection page could have a reference to this page so it's easier to find it out.

I was initially confused by docs/ vs. packages/docs/ in the monorepo. Maybe rename docs/ to contributing/.

Yeah, those are the developer docs. I would prefer to keep the directory as it is, but I don't mind clarifying in the introduction page where the user docs are placed.

@camsteffen
Copy link
Author

camsteffen commented Jan 5, 2025

...I would prefer to keep it the way it is.

Even this part?

WARNING
Experimental decorators and Emit Decorator Metadata options must be enabled in order to use this library.


Yeah, it's established in the API docs, but I honestly believe the Injection page could have a reference to this page so it's easier to find it out.

Hmm, to be honest that section isn't very helpful to me. I would want "When is injectable mandatory?" to tell me what InversifyJS features are enabled by using the decorator.

I think it's pretty common for documentation to have a Guide section and an API section. So honestly when I see Fundamentals and API, I think "oh Fundamentals must be Guide".

@notaphplover
Copy link
Member

notaphplover commented Jan 5, 2025

Hey @camsteffen

Even this part?

Ohh, no, I mean keeping developer docs directory.

All right, I am open to try another approach, I would love to see your proposal in the PR. As long as we cover the concepts already covered I don't mind changing the fundamentals section.

To be honest I've been struggling to make docs readable and easy to understand. As an english as a second language speaker, I do appreciate the feedback of a native speaker to polish docs. As a maintained who have a full understanding of the library, almost everything seems readable and easy to understand, so I'm more than glad to reive this feedback. You're more than welcome to make your proposal ☺️ in a PR.

If you need any sort of clarification we can discuss it in the PR or even schedule a meeting

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.

2 participants