Build system

[realamirhe]

Please double-check the process locally and release on the RC tag in npm

[sourcelevel-bot]

Hello, @realamirhe! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

[vizipi]

Pull request analysis by VIZIPI

Below you will find who is the most qualified team member to review your code.
This analysis includes his/her work on the code included in this Pull request, in addition to their experience in code affected by these changes ( partly found within the list of potential missing files below )   Feedback always welcome

Reviewers with knowledge related to these changes

Match %	Person	Commit Count	Common Files
87.88 %	Chintan Prajapati	214	29

---

Potential missing files from this Pull request

files commonly committed with a subset of this pr, but not committed this time. (click to collapse)

File	Percentile	rate
src/Plyr.tsx	50.00 %	1 out of 2 times

---

Committed file ranks

(click to expand)

56.93%[example/src/custom-hls-player.tsx]

93.67%[config/webpack.js]

81.02%[example/vite.config.ts]

73.19%[tests/Plyr.test.tsx]

81.02%[config/fileTransform.js]

85.84%[config/cssTransform.js]

73.19%[example/src/custom-audio-player.tsx]

99.70%[package-lock.json]

99.40%[package.json]

97.89%[rollup.config.js]

[realamirhe]

The problem of the ci seems to be because of GitHub actions node_modules cache.

It is totally okay locally, and I believe if we switch to yarn it would be okay too
but in case we want to stick to npm, Do you have any idea how to fix it @chintan9 ?
https://stackoverflow.com/a/49505612/10321531

[chintan9]

The problem of the ci seems to be because of GitHub actions node_modules cache.

It is totally okay locally, and I believe if we switch to yarn it would be okay too but in case we want to stick to npm, Do you have any idea how to fix it @chintan9 ? https://stackoverflow.com/a/49505612/10321531

(!) Plugin typescript: @rollup/plugin-typescript TS2322: Type '(_: any, params: PlyrConfigurationProps) => PlyrJS' is not assignable to type 'Instantiate<Plyr, HTMLVideoElement, PlyrConfigurationProps>'.
  Types of parameters 'params' and 'params' are incompatible.
    Type 'PlyrConfigurationProps | undefined' is not assignable to type 'PlyrConfigurationProps'.
      Type 'undefined' is not assignable to type 'PlyrConfigurationProps'.
src/index.tsx: (74:7)

74       instantiate,
         ~~~~~~~~~~~

  node_modules/react-aptor/use-aptor.d.ts:8:5
    8     instantiate: Instantiate<TInstance, TElement, TParams>;
          ~~~~~~~~~~~
    The expected type comes from property 'instantiate' which is declared here on type 'AptorConfiguration<Plyr, HTMLVideoElement, PlyrConfigurationProps>'
    
    ```

[chintan9]

can we use SWC instead of esbuild?

[realamirhe]

can we use SWC instead of esbuild?

I tried that, but I took too much time to install and got failed at the end.

If you can install
@swc/core and @swc/jest and update the package-lock.json;
I will do the setup on my own

[realamirhe]

The problem of the ci seems to be because of GitHub actions node_modules cache.

It is totally okay locally, and I believe if we switch to yarn it would be okay too but in case we want to stick to npm, Do you have any idea how to fix it @chintan9 ? https://stackoverflow.com/a/49505612/10321531

(!) Plugin typescript: @rollup/plugin-typescript TS2322: Type '(_: any, params: PlyrConfigurationProps) => PlyrJS' is not assignable to type 'Instantiate<Plyr, HTMLVideoElement, PlyrConfigurationProps>'.
  Types of parameters 'params' and 'params' are incompatible.
    Type 'PlyrConfigurationProps | undefined' is not assignable to type 'PlyrConfigurationProps'.
      Type 'undefined' is not assignable to type 'PlyrConfigurationProps'.
src/index.tsx: (74:7)

74       instantiate,
         ~~~~~~~~~~~

  node_modules/react-aptor/use-aptor.d.ts:8:5
    8     instantiate: Instantiate<TInstance, TElement, TParams>;
          ~~~~~~~~~~~
    The expected type comes from property 'instantiate' which is declared here on type 'AptorConfiguration<Plyr, HTMLVideoElement, PlyrConfigurationProps>'
    
    ```

I'm pretty sure, I fixed this😅,
Okay let me take another look to it

[realamirhe]

Hey @chintan9, I fixed the type Instantiate Bug, it was due to a type limitation, now we have 3 warnings but there is a completely valid statement, and have a plan to fix them soon.

/plyr-react/src/index.tsx
  37:42  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion
  38:7   warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion
  38:37  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion

The problem with the plyr

(!) Plugin typescript: @rollup/plugin-typescript TS2307: Cannot find module 'plyr' or its corresponding type declarations.

Seems to exist, because of the minimum version of our npm in the pacakge.json engine
I was using the node: v16.14.0 and npm: 8.3.1 so I didn't have any issues.

Here are some links which can be helpful

• stack overflow
• install peer deps
• Presenting v7.0.0 of the npm CLI

[realamirhe]

Now we have another problem with the react testing library, I remember I solved that on my side (locally) via --legacy-peer-deps which is not a good option.

I'm going to fix the react testing library react peer deps after Monday.
And we might be able to publish the latest soon
@chintan9

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

No Coverage information
0.0% Duplication

[sourcelevel-bot]

SourceLevel has finished reviewing this Pull Request and has found:

• 1 possible new issue (including those that may have been commented here).
• 1 fixed issue! 🎉

See more details about this review.

[realamirhe]

Fixed the conflicts and build errors, all kudos to dear gitpod :)

[realamirhe]

@chintan9 although I love to pass all these commit to master on my name?
I changed the package-lock.json and it faced conflicts, so I was forced to merge master into the build-system, so it is up to you what to do, but it seems the first is more professional action.

1. squash and merge

2. Merge pull request