feat!: use wrapped functions

[Shinigami92]

Draft for #1250

... it could be that this is only an internal soft breaking change :thinking:

---

So this PR has two benefits:

1. We get rid of the constructor hack

  constructor(private readonly faker: Faker) {
    // Bind `this` so namespaced is working correctly
    for (const name of Object.getOwnPropertyNames(<Module>.prototype)) {
      if (name === 'constructor' || typeof this[name] !== 'function') {
        continue;
      }
      this[name] = this[name].bind(this);
    }
  }

2. We are save in the future for misusing this and bindings of this

New downsides for now: it gets a little bit more complex for new contributors to add a new function as they would need to declare the TypeScript definition in the module's interface and implement it in the module wrapped-fucntion.

[codecov[bot]]

Codecov Report

Merging #1254 (41db9e5) into main (462ee5a) will decrease coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1254      +/-   ##
==========================================
- Coverage   99.62%   99.62%   -0.01%     
==========================================
  Files        2156     2156              
  Lines      237025   237035      +10     
  Branches     1006     1002       -4     
==========================================
+ Hits       236126   236135       +9     
- Misses        878      879       +1     
  Partials       21       21              

Impacted Files	Coverage Δ
src/faker.ts	100.00% <100.00%> (ø)
src/modules/git/index.ts	100.00% <100.00%> (ø)
src/modules/finance/index.ts	99.77% <0.00%> (-0.23%)	:arrow_down:

[Minozzzi]

I like the idea of applying the interface to the git export function. Something like git export function(faker: Faker): Git. Therefore, an error will be displayed when there are not all methods. I know it is already being applied to faker.ts. But I think it would be more of a security in development. What do you think about it?

[Shinigami92]

I like the idea of applying the interface to the git export function. Something like git export function(faker: Faker): Git. Therefore, an error will be displayed when there are not all methods. I know it is already being applied to faker.ts. But I think it would be more of a security in development. What do you think about it?

lel that was originally the idea why I extracted the interface :facepalm: But eslint didn't show me the warning anymore because I ignored it :rofl:

[xDivisionByZerox]

I don't like this approach at all. TBH I don't see the benefits you described when looking at the extra declaration work.

Furthermore, what do you change at all with this approach? Isn't that the same thing that would happen for a js class under the hood?

I think using classes is perfectly fine for us and so is using the "constructor hack", IMO. I guess the real problem is with our modularization strategy.

[Shinigami92]

I don't like this approach at all. TBH I don't see the benefits you described when looking at the extra declaration work.

Furthermore, what do you change at all with this approach? Isn't that the same thing that would happen for a js class under the hood?

I think using classes is perfectly fine for us and so is using the "constructor hack", IMO. I guess the real problem is with our modularization strategy.

Yeah, you raise immediately all the questions I answered in the last meeting :smile:

So first: re-read the PRs description text (I updated it a bit)

1. the constructor hack is removed
2. we can now use "private"/"not exported" functions inside modules
3. this is theoretically a pre-phase towards moving completely to only exported functions so in the (no so near) future we would like to have something like this:

src/
  modules/
    git/
      index.ts
      branch.ts
      commitEntry.ts
      <...>.ts

where index.ts is only:

import { branch } from './branch'
import { commitEntry } from './commitEntry'

export interface Git {
  ...
}

export const git: Git = {
  branch,
  commitEntry,
  ...
}

export {
  branch,
  commitEntry,
  ...
}

and the functions would be extracted to their specific files

The benefit of that would be that we then make use of tree-shaking

BUT: we need to find a way of how we pass the faker instance around and reusing the same seed sequence (not solved yet) @ST-DDT proposed a solution using factories for the functions, but we didn't tested that out

Any further questions right now?

[xDivisionByZerox]

3. this is theoretically a pre-phase towards moving completely to only exported functions so in the (no so near) future we would like to have something like this:

Ok, this is exactly what I had in mind. All methods are exported by themself and as a collection/module from the index.ts file. You guys work just like me.

I still don't see why this step is required now, but it doesn't introduce any breaking changes, so it should be fine.

[xDivisionByZerox]

export { branch, commitEntry, ... }

I would not suggest exporting specific functions from the index.ts file since that can lead to side-effects and could potentially break tree-shaking.

Just wanted to leave that here.

[xDivisionByZerox]

2. we can now use "private"/"not exported" functions inside modules

We can do this already. Not private, but I don't see a problem with the approach we have so far.

[import-brain]

New downsides for now: it gets a little bit more complex for new contributors to add a new function as they would need to declare the TypeScript definition in the module's interface and implement it in the module wrapped-fucntion.

If we do go this route, we'll definitely need documentation on how to implement a function in the new system.

[Shinigami92]

If we do go this route, we'll definitely need documentation on how to implement a function in the new system.

Absolutely, but as I now dig into it and start experimenting/implementing/working with it, I start to feel more and more like @xDivisionByZerox We only solve something real with it if we reach tree-shaking support (not bundled together modules that the user can use) otherwise it's only a "Disimprovement" ("Verschlimmbesserung") for the source code

[Shinigami92]

Closing these experiments for now

[ST-DDT]

@Shinigami92 Do you still need the branch? If not, please delete it.

Commit-ID: 41db9e5727e1f5d275e887ee796219cbea8d1003

[Shinigami92]

Yes, please do not delete them right now so we have a copy and look into it if needed I just wanted to clean the PR section as we would need to rewrite them anyway as they got to stale