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

chore: drop console.log from production builds #1012

Closed
wants to merge 2 commits into from

Conversation

flexchar
Copy link

@flexchar flexchar commented Jun 23, 2024

Details

This PR enables drop_console option in swc minifier.

I tried to add support for custom .swcrc but I couldn't get it work in any way. It would be amazing if people could configure that. But I also think it's ok to not have console log in production. What do you think?

Options docs: https://swc.rs/docs/usage/core#options
Compress docs: https://swc.rs/docs/configuration/minification#jscminifycompress

Related: #542

Code of Conduct

  • I agree to follow this project's Code of Conduct
  • I agree to license this contribution under the MIT LICENSE
  • I checked the current PR for duplication.

Contacts

  • (OPTIONAL) Discord ID:

If your PR is accepted, we will award you with the Contributor role on Discord server.

To join the server, visit: https://www.plasmo.com/s/d

Comment on lines +188 to +192
all_frames: booleanSchema,
world: {
type: "string",
enum: ["MAIN", "ISOLATED"]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flexchar - might need to double check if this is available on Firefox yet. If not, let's add this, but also keep the old behavior for cross-browser compatibility I think :-?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually would be best to do the drop console in a separate PR to merge quickly, then we can do the main world fix later :-?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do that!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow wat?... so compress: true only doesn't drop the console?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It's not under the defaults. I included the link to the docs :)

@louisgv louisgv changed the title Drop console.log from production buils chore: drop console.log from production builds Jul 1, 2024
Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff needs to be a isolated to what it's trying to do.

@mathieudutour
Copy link
Contributor

I don't particularly agree with this change: there are valid reasons to want logs in prod build. The way to usually deal with it is to have a logging utility where you choose which level of logs you want to keep in prod build based on a variable in process.env (there are tons of utilities like that, https://github.com/debug-js/debug is one of them)

@flexchar
Copy link
Author

flexchar commented Jul 3, 2024

@mathieudutour that's a good point, that's why I added an open question to my PR. Ideally i would like to support .swcrc file to let people customize but after hours I just couldn't get it to work. I think it is because it is not supported when contents are passed as a string. Do you agree @louisgv?

It could also be a CLI option but it feels wrong. It feels that something like this should live in a dedicated config file.

@louisgv
Copy link
Contributor

louisgv commented Jul 4, 2024

Hmm @flexchar yeah I think most sane solution would be to suggest end-user to leverage env vars. Parcel prunes dead branches. Thus:

if (process.env.NODE_ENV === 'development') {
     console.log("etc")
}

Will be pruned if it's production

@louisgv louisgv closed this Jul 4, 2024
@flexchar
Copy link
Author

flexchar commented Jul 5, 2024

Oh, I didn't know this existed. I did have simple function log() that had this in the body but Parcel wasn't smart enough to remove those. This is because SWC processes each file individually so it is not aware of how imported functions behave. This is something I am generally really not fond of the way it work. It also prevents some other features. Hence the inspiration for this.

I am definitely less experienced in the context of custom parcel + swc. Do you think there is a relatively easy way to provide config file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants