faker icon indicating copy to clipboard operation
faker copied to clipboard

infra(ci): run against playground

Open mshima opened this issue 1 year ago โ€ข 19 comments

Main types index.d.ts doesnโ€™t exist. Fix types to match '.' exports.

ref: https://github.com/jhipster/generator-jhipster/pull/27179

mshima avatar Sep 07 '24 14:09 mshima

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
Latest commit 356e3ae35fdd041740ceee66dee36b070087e93b
Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66e03332d2c0220008ede2e5
Deploy Preview https://deploy-preview-3095.fakerjs.dev
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Sep 07 '24 14:09 netlify[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.97%. Comparing base (18ab2c7) to head (356e3ae). Report is 174 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3095      +/-   ##
==========================================
+ Coverage   99.96%   99.97%   +0.01%     
==========================================
  Files        2776     2776              
  Lines      226260   226260              
  Branches      945      591     -354     
==========================================
+ Hits       226183   226207      +24     
+ Misses         77       53      -24     

see 1 file with indirect coverage changes

codecov[bot] avatar Sep 07 '24 14:09 codecov[bot]

Same fix is applied in https://github.com/faker-js/faker/pull/3094. Alternative fix is applied in https://github.com/faker-js/faker/pull/3093 (removes types from root)

The above PRs tries to fix CommonJS types too, while this fixes esm types.

mshima avatar Sep 07 '24 15:09 mshima

you can checkout https://github.com/faker-js/playground and test your changes

Shinigami92 avatar Sep 07 '24 15:09 Shinigami92

you can checkout https://github.com/faker-js/playground and test your changes

Why donโ€™t integrate playground on ci tests?

mshima avatar Sep 07 '24 15:09 mshima

you can checkout faker-js/playground and test your changes

Why donโ€™t integrate playground on ci tests?

PR welcome, go and try for it

Shinigami92 avatar Sep 07 '24 15:09 Shinigami92

@mshima thanks, this is a huge step forward and I wished for something like this a long time โค๏ธ you need to rebind/relink the repo faker to the playground test run so something like modifying pnpm override before installing the playground deps

Shinigami92 avatar Sep 07 '24 17:09 Shinigami92

@Shinigami92 I am on mobile now, so I cannot rebase. I think itโ€™s only missing steps names now. Feel free to modify the PR.

Playground is passing ๐Ÿš€

mshima avatar Sep 07 '24 17:09 mshima

Playground seems to be missing pure node ts samples.

mshima avatar Sep 07 '24 18:09 mshima

AND NOW we really see the result! ๐ŸŽ‰

image

Right now, this behavior is correct! because we have this issue

Shinigami92 avatar Sep 07 '24 18:09 Shinigami92

For some reason mapping types to non existing index.d.ts fixes playground.

In the end it just adds the playground workflow

mshima avatar Sep 07 '24 19:09 mshima

Please note that this will create a bi-directional dependency between two separate repositories.

ST-DDT avatar Sep 08 '24 13:09 ST-DDT

Please note that this will create a bi-directional dependency between two separate repositories.

Do you see any problem with this? IMO it's fine, because playground is in out org as well.

Shinigami92 avatar Sep 08 '24 13:09 Shinigami92

IMO running playground in CI:

  • reduces maintenance burden by avoiding regressions and requiring manual checks: https://github.com/faker-js/faker/pull/3093#issuecomment-2339729022.
  • improves contribution experience by not having to ask to manually check against playground: https://github.com/faker-js/faker/pull/3095#issuecomment-2335533878

We could do that but it has a side effect, that all features that are used in the playground that might experience breaking changes need to be updated to the new feature before it is available as a dependency. This is also the case for the releasenPR but that those are even rarer.

  • the workflow can be ignored if it's not required to pass.
  • contributor will be warned about breaking changes or unwanted breaking changes.

Moving playground to the main repository can be considered too.

mshima avatar Sep 10 '24 11:09 mshima

  • the workflow can be ignored if it's not required to pass.
  • contributor will be warned about breaking changes or unwanted breaking changes.

I agree ๐Ÿ‘ we can mark the CI check as "not required" and if it breaks something, we are just "informed"

Shinigami92 avatar Sep 10 '24 11:09 Shinigami92

I agree ๐Ÿ‘ we can mark the CI check as "not required" and if it breaks something, we are just "informed"

Please note that that will only work once and will cause confusion for future PRs (that will fail the CI even if they didn't break anything because next is already "broken").

I would rather move the playgrounds inside the main repo to avoid these bidirectional dependencies.

ST-DDT avatar Sep 12 '24 14:09 ST-DDT

Team Decision

We don't want to add bi-directional dependencies between the playground and the faker repo. We think that moving the playground to the main repo is the best solution for that, but that would require extensive work e.g. converting the repo into a mono-repo, what we don't want to do yet.

We would like to still run the pipeline on demand somehow e.g. using a comment, but we are currently unsure about the exact expectation we have for the pipeline. We will talk about this again in a future team meeting.

ST-DDT avatar Sep 12 '24 15:09 ST-DDT

If you have an idea, on how to move the playground to the main repo feel free to open a PR with a suggestion.

ST-DDT avatar Sep 12 '24 16:09 ST-DDT

We are looking into restructuring our project setup to solve this issue. For now, you don't have to update this PR. Thanks for bringing this to our attention.

ST-DDT avatar Sep 19 '24 15:09 ST-DDT