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

Cross-module dead code elimination of constant doesn't work with defines #2061

Closed
vlovich opened this issue Feb 27, 2022 · 5 comments
Closed

Comments

@vlovich
Copy link

vlovich commented Feb 27, 2022

Hmm.... I think I must be doing something wrong. I'm trying to take advantage of #1981 in 0.14.19. have a constants.ts file like this:

declare global {
  const __IS_PRODUCTION_BUILD__: boolean
}
export const isProductionBuild = __IS_PRODUCTION_BUILD__

In build.mjs I have:

...
define: {
  __IS_PRODUCTION_BUILD__: process.env.NODE_ENV === 'production'
}
...

I've used isProductionBuild in my code like so:

// Workaround for https://github.com/cloudflare/miniflare/issues/191
import { isProductionBuild } from './constants'

function isDateInstance(value: any): value is Date {
  if (!isProductionBuild) {
    return value instanceof Date || (typeof value === 'object' && value.constructor.name === 'Date')
  }
  return value instanceof Date
}

function someFunction(value: any) {
   if (isDateInstance(value)) {
     ...
   }
}

I would expect that the conditional if (isDateInstance(value)) gets minified to if(value instanceof Date) in a production build. Instead I see:

function KA(e){return!0?e instanceof Date:e instanceof Date||e instanceof Object&&e.constructor.name==="Date"}

and later

if(KA(t))
@evanw
Copy link
Owner

evanw commented Feb 27, 2022

General function inlining like this is not an optimization that esbuild does. This is documented here: https://esbuild.github.io/api/#minify-considerations. You are welcome to use another tool instead if you need this optimization.

@vlovich
Copy link
Author

vlovich commented Feb 27, 2022

Fine, but it's still not inlining the constant as intended right?.

function isWorkaroundDateInstance(value: any): value is Date {
   return value instanceof Date || (typeof value === 'object' && value.constructor.name === 'Date')
}

if (value instanceof Date || (!isProductionBuild && isWorkaroundDateInstance(value) {
}

becomes

if(t instanceof Date||!!0&&KA(t))

rather than

if(t instanceof Date)

It's failing to recognize that !!0&&anything can be stripped because it will never evaluate to true.

@vlovich
Copy link
Author

vlovich commented Feb 27, 2022

FWIW this behaves as intended:

if (value instanceof Date || (!__IS_PRODUCTION_BUILD__ && isWorkaroundDateInstance(value) {
}

so this does seem to be a constant inlining problem.

@evanw
Copy link
Owner

evanw commented Feb 27, 2022

This is also in the documentation. What you are referring to is the "Cross-statement constant propagation" bullet point. You can read more about why this limitation exists here: #2006 (comment).

@evanw
Copy link
Owner

evanw commented Mar 3, 2022

This is a known design limitation. The best way to do this is to avoid indirection through another variable, as described above. Closing as "won't fix."

@evanw evanw closed this as completed Mar 3, 2022
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

No branches or pull requests

2 participants