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

Mark css files as side effects #963

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

Conversation

desfero
Copy link

@desfero desfero commented Sep 26, 2022

Right now production build of parcel (and all other bundlers that take into account sideEffects flag) removes the css files completely from the bundle.

Specifying explicitly that css has side effects would solve the issue.

@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Sep 26, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@vizipi
Copy link

vizipi bot commented Sep 26, 2022

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
100.00% Chintan Prajapati 103 1
100.00% amirHossein-Ebrahimi 29 1
100.00% tractorcow 1 1

Potential missing files from this Pull request

files commonly committed with a subset of this pr, but not committed this time. (click to collapse)
FilePercentilerate
package-lock.json91.20%808 out of 886 times

Committed file ranks

(click to expand)
  • 99.49%[package.json]
  • @vizipi vizipi bot requested a review from chintan9 September 26, 2022 11:10
    @cesarenaldi
    Copy link

    Any update on this PR? Can it be merged?

    @ghost
    Copy link

    ghost commented Dec 2, 2022

    👇 Click on the image for a new way to code review

    Review these changes using an interactive CodeSee Map

    Legend

    CodeSee Map legend

    @chintan9 chintan9 requested a review from realamirhe December 2, 2022 02:25
    @realamirhe
    Copy link
    Collaborator

    Hey @desfero nad @cesarenaldi

    We copy the CSS file after building in postbuild our scripts

    plyr-react/package.json

    Lines 52 to 56 in 0e6459f

    "postbuild": "npm run copy",
    "precopy": "shx cp -r dist/src/* dist/esm && shx cp -r dist/src/* dist && shx rm -rf dist/src && shx rm -rf dist/{src,tests}",
    "copy": "concurrently -m 8 'npm:copy:*'",
    "copy:package-json": "shx cp package.json dist && json -I -f dist/package.json -e 'this.private=false; this.devDependencies=undefined; this.scripts=undefined; this.publishConfig=undefined'",
    "copy:static-content": "shx cp README.md LICENSE dist",

    I checked the latest version of dist.tarball from npm view command and it seems that we have the plyr.css in our final build.

    I always thought that the "sideEffects" is mostly going to be used for tree-shaking.

    @sonarqubecloud
    Copy link

    Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

    Bug A 0 Bugs
    Vulnerability A 0 Vulnerabilities
    Security Hotspot A 0 Security Hotspots
    Code Smell A 0 Code Smells

    No Coverage information No Coverage information
    0.0% 0.0% Duplication

    @realamirhe
    Copy link
    Collaborator

    it changes the side effect property for all end devs so it needs more investigation before merging!

    @realamirhe realamirhe self-assigned this Dec 19, 2022
    @realamirhe realamirhe added 🌟 Needs more Approvals Pull Request needs more approvals ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Dec 19, 2022
    @trafico-bot trafico-bot bot removed 🌟 Needs more Approvals Pull Request needs more approvals ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Apr 17, 2023
    @sonarqubecloud
    Copy link

    Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

    Bug A 0 Bugs
    Vulnerability A 0 Vulnerabilities
    Security Hotspot A 0 Security Hotspots
    Code Smell A 0 Code Smells

    No Coverage information No Coverage information
    0.0% 0.0% Duplication

    @al-petrushin
    Copy link

    Any update here? 🤔

    Copy link

    Quality Gate Passed Quality Gate passed

    Kudos, no new issues were introduced!

    0 New issues
    0 Security Hotspots
    No data about Coverage
    0.0% Duplication on New Code

    See analysis details on SonarCloud

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    🔍 Ready for Review Pull Request is not reviewed yet size/XS
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants