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

Support object literals for define #581

Closed
jbms opened this issue Dec 6, 2020 · 3 comments
Closed

Support object literals for define #581

jbms opened this issue Dec 6, 2020 · 3 comments

Comments

@jbms
Copy link

jbms commented Dec 6, 2020

Currently the define transform only supports identifiers, strings, numbers, boolean, and null. It would be convenient to also support object literals, or ideally arbitrary JavaScript expressions. Object literals in particular are an important use case for me.

Is there a reason that can't be supported?

jbms added a commit to jbms/esbuild that referenced this issue Dec 6, 2020
@evanw
Copy link
Owner

evanw commented Dec 6, 2020

The reason this isn't supported is that doing this is an anti-pattern. The define feature replaces the expression with the value inline. Replacing each instance of an identifier with an object literal has the potential to bloat the generated code by a lot. This is almost certainly undesirable.

For example, replacing process.env with an object literal containing the whole environment would result in a lot of unnecessary code added to many open-source libraries that access process.env.NODE_ENV. It's better to use
the define feature for each individual field instead since that avoids code bloat.

If you still want to replace an identifier with an object literal, you should probably use the inject feature instead. That replaces the expression with a reference to an exported value. That way the object literal only appears once in the generated code. You can read more about the inject feature here: https://esbuild.github.io/api/#inject.

@jbms
Copy link
Author

jbms commented Dec 6, 2020

That is a good point that I didn't really give a lot of thought to.

Still it seems like something that could be warned about in the documentation, rather than prohibited altogether. The same issue does also apply to long strings.

For my use case, I only really reference the identifier twice, and one of them is a typeof expression that could theoretically be optimized out:
https://github.com/google/neuroglancer/blob/05471fc9d02335915d3697f92189f33c2e557624/src/neuroglancer/datasource/brainmaps/register_default.ts#L28

Still, I agree that the behavior of inject is better, though it is slightly less convenient to have to write a temporary file. It seems that there could be a variant of definethat behaves like inject, in that it defines a uniquely named constant with the specified value, then substitutes the specified dotted identifier sequence with a reference to that constant.

@koshic
Copy link

koshic commented Dec 25, 2020

@evanw , for my use case it's need to pass a simple structures inside bundled code (AWS CDK + Lambda@Edge which highly restricted in terms of configuration). There are 3 ways to handle this:

  1. Use JSON's stringify / parse pair. Not bad at all, but requires to write small boilerplate for each define.
  2. Use imports with all-possible-values-map and define string key to get required part of data. Less magic (looks very similar to env vars flow), but it means my bundle will include data which will never used.
  3. Temporary file generation and inject. There are a lot of issues: tmp files generation & management, a LOT of magic, and custom wrapper to generate esbuild options => too complicated solution against 1 or 2.

Now I prefer option 2, but code is still not ideal.

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