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

Use wrapped functions instead of classes for modules #1250

Closed
Shinigami92 opened this issue Aug 9, 2022 · 3 comments
Closed

Use wrapped functions instead of classes for modules #1250

Shinigami92 opened this issue Aug 9, 2022 · 3 comments
Assignees
Labels
breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs
Milestone

Comments

@Shinigami92
Copy link
Member

Clear and concise description of the problem

Currently we need to bind this in the constructor of each module to support stuff like

const color = faker.color
color.rgb()
// and
const rgb = faker.color.rgb
rgb()

Suggested solution

Instead of using js classes we could instead use wrapped functions like e.g. composables are working in Vue / VueUse

Alternative

No response

Additional context

This might be a breaking change as with that you would not create a new class via new Faker(...) anymore
Also we need to benchmark if there would be performance regressions or benefits 🤔

I would like to setup a PR if this proposal gets accepted by the team

@Shinigami92 Shinigami92 added the s: pending triage Pending Triage label Aug 9, 2022
@Shinigami92 Shinigami92 self-assigned this Aug 9, 2022
@ST-DDT ST-DDT added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release labels Aug 9, 2022
@import-brain import-brain added s: needs decision Needs team/maintainer decision and removed s: pending triage Pending Triage labels Aug 9, 2022
This was referenced Aug 10, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Mar 16, 2023

Team Decision

We will think about this again when we actually start the tree shaking.

@ST-DDT
Copy link
Member

ST-DDT commented Feb 15, 2024

I wrote a proposal for a design for this one in #2667 (because it is quite long)

@ST-DDT
Copy link
Member

ST-DDT commented Feb 29, 2024

@ST-DDT ST-DDT closed this as completed Feb 29, 2024
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs
Projects
No open projects
Status: No status
Development

No branches or pull requests

3 participants