search

solana-labs / solana-web3.js

Solana JavaScript SDK
https://solana-labs.github.io/solana-web3.js
MIT License
1.97k stars 792 forks source link

Replace the `__DEV__` flag with literally `process.env.NODE_ENV` in the built source #2840

Closed steveluscher closed 1 week ago

steveluscher commented 2 weeks ago

Summary

Previously, we were using the hair-brained inject feature of ESBuild to try to replace all instances of __DEV__ with something resembling process.env.NODE_ENV === 'development'. This just didn't work.

In this PR we replace it with the literal process.env.NODE_ENV === 'development', pre-sourcemaps, and only inline the actual value of NODE_ENV in iife bundles.

Test Plan

Consider the application in #2817. pnpm build it.

Before

~/src/solana-web3.js-git/examples/react-app$ pnpm build

> @solana/example-react-appz@0.0.0 build /home/sol/src/solana-web3.js-git/examples/react-app
> tsc && vite build

vite v5.3.0 building for production...
✓ 382 modules transformed.
dist/index.html                   0.51 kB │ gzip:   0.32 kB
dist/assets/index-CNo840Gp.css  690.33 kB │ gzip:  81.40 kB
dist/assets/index-OpUl_uG9.js   398.96 kB │ gzip: 126.36 kB
✓ built in 2.08s

After (5% reduction in total application bundle size)

~/src/solana-web3.js-git/examples/react-app$ pnpm build

> @solana/example-react-appz@0.0.0 build /home/sol/src/solana-web3.js-git/examples/react-app
> tsc && vite build

vite v5.3.0 building for production...
✓ 382 modules transformed.
dist/index.html                   0.51 kB │ gzip:   0.32 kB
dist/assets/index-CNo840Gp.css  690.33 kB │ gzip:  81.40 kB
dist/assets/index-DBjRBfM_.js   379.01 kB │ gzip: 119.86 kB
✓ built in 2.11s
changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: 88d5b025ccdb2375c1bbcb9fe9e27f805fd3a1fe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

steveluscher commented 2 weeks ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @steveluscher and the rest of your teammates on Graphite Graphite

lorisleiva commented 1 week ago

@steveluscher Just catching up on that. Does that mean I need to replicate that change to the program repos? Since we also use a __DEV__ variable on the generated code to avoid bundling error messages.

steveluscher commented 1 week ago

Definitely! It basically doesn't work with the env-shim method. This method results in actual DCE.

lorisleiva commented 1 week ago

How would you feel about process.env.NODE_ENV === 'development' being generated directly by Kinobi instead of using a __DEV__ variable that needs to be replaced at build time? The issue with the latter is that anyone the generates code using Kinobi now needs to implement the same pre-processing build step which is a concern we've already received from the community.

github-actions[bot] commented 1 week ago

:tada: This PR is included in version 1.93.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

lorisleiva commented 1 week ago

@steveluscher Just bumping that in case the GitHub bot made you mark this as done. 😇

steveluscher commented 1 week ago

How would you feel about process.env.NODE_ENV === 'development' being generated directly by Kinobi instead of using a DEV variable that needs to be replaced at build time?

Much better. This is what web3.js does now, essentially. Nobody except for us developers ever see __DEV__; it's just a shorthand for us that gets replaced with { development flag expression du jour }.