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

✨ add support for babel-plugin-macros #64

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

intellild
Copy link

@intellild intellild commented Feb 24, 2022

support babel-plugin-macros
example:

import metadata from "babel-plugin-transform-typescript-metadata/lib/macro";

@metadata
class A {}

@intellild intellild changed the title ✨ Macro support ✨ add support for babebl-plugin-macros Feb 24, 2022
@intellild intellild changed the title ✨ add support for babebl-plugin-macros ✨ add support for babel-plugin-macros Feb 24, 2022
@intellild
Copy link
Author

Hi @leonardfactory can you look at this ?

Copy link
Owner

@leonardfactory leonardfactory left a comment

Choose a reason for hiding this comment

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

Hi and thank you for your contribution! I left my two cents here

package.json Outdated
Comment on lines 77 to 87
"peerDependencies": {
"babel-plugin-macros": "^3.1.0"
},
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a bit worried about this, since babel-plugin-macro is not strictly a dependency, but an optional one. An idea would be to use peerDependenciesMeta in order to specify they're optional, even if this would require additional checking in actual code. Can you take a look at this?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds reasonable.

@intellild
Copy link
Author

@leonardfactory I made the babel-plugin-macros dependency optional

@leonardfactory
Copy link
Owner

Hi @intellild, I didn't have time to analyze this correctly before; now I'm wondering which use case you are trying to cover here. I'm genuinely curious about your proposal, I'll try to explain my point of view. The plugin works well when you need to work with Reflect.metadata outputs, and this usually requires every decorated class / property / etc. to be processed: in this situation so, manually decorating everything could be cumbersome, and it can be considered an implicit side effect and, like babel-plugin-macros already states:

Explicit is often a better pattern than implicit because it requires others to understand how things are globally configured. This is in this spirit are babel-plugin-macros designed. However, some things do need to be implicit, and those kinds of babel plugins can't be turned into macros.

Furtermore this plugin tries to emulate (with a best-effort strategy) the official TypeScript feature --emitDecoratorMetadata, which is implict too.

Given all of that, I'm curious about the macro usage and I'd like to hear back from you before including it in the package – I'm just worried it could bring a bit of confusion to future and existing users.

@intellild
Copy link
Author

@leonardfactory
There are two problems.

  • The project boilerplate may not support adding babel plugins, such as create-react-app
  • I don't want to affect some one else's code in a huge project which maybe more than one hundred people is working on, while the macro marks classes that need metadata explicitly

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