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

Template: don't call lodashTemplate if define param is undefined #46

Conversation

denis-ok
Copy link
Contributor

@denis-ok denis-ok commented Aug 3, 2023

Description

I attempt to use esbuild-plugin-html with custom HTML templates for different locales. HTML templates contain a script tag where the object with translation messages for react-intl is defined. Example:

<script>
  globalThis["__intlMessages__"] = {"1d7f4940074ad8704abbaf70c1316258":"${amount} / 年"}
</script>

The problem is that translation message contain $ character with a placeholder for a runtime value inside of curly braces. Example: "${amount} / 年" (it will be $99 / 年 at runtime). ${amount} is a legit target of lodash template function, but I got esbuild error because amount value is missing in define param object. I don't use define param in esbuild-plugin-html and I don't need to replace anything in my HTML templates.

Currently, it produces a build error:

/home/me/monorepo/node_modules/lodash.template/index.js:1550:11: ERROR: [plugin: esbuild-html-plugin] amount is not defined
    at failureErrorWithLog (/home/me/monorepo/node_modules/esbuild/lib/main.js:1649:15)
    at /home/me/monorepo/node_modules/esbuild/lib/main.js:1058:25
    at /home/me/monorepo/node_modules/esbuild/lib/main.js:1525:9 {
  errors: [
    {
      id: '',
      pluginName: 'esbuild-html-plugin',
      text: 'amount is not defined',
      location: {
        file: '/home/me/monorepo/node_modules/lodash.template/index.js',
        namespace: 'file',
        line: 1550,
        column: 11,
        length: 0,
        lineText: "    return Function(importsKeys, sourceURL + 'return ' + source)\n" +
          '    at eval (eval at <anonymous> (/home/me/monorepo/node_modules/lodash.template/index.js:1550:12), <anonymous>:8:10)\n' +
          '    at renderTemplate (/home/me/monorepo/node_modules/@craftamap/esbuild-plugin-html/lib/cjs/index.js:103:16)\n' +
          '    at async /home/me/monorepo/node_modules/@craftamap/esbuild-plugin-html/lib/cjs/index.js:246:46\n' +
          '    at async /home/me/monorepo/node_modules/esbuild/lib/main.js:1494:27',
        suggestion: ''
      },
      notes: [
        {
          text: 'This error came from the "onEnd" callback registered here:',
          location: [Object]
        }
      ],
      detail: 0
    }
  ],

Solution

I propose to simply don't call lodashTemplate function if define param is undefined.

Another possible option is to introduce an extra param like useLodashTemplate: bool or something, but I think my suggestion is simpler and just works.

Overall, I think it's a good idea to avoid calling lodashTemplate function at all when it isn't needed as a performance optimization.

@craftamap
Copy link
Owner

Thanks for this contribution. :)

I think skipping lodash _.template if define is not set is a reasonable thing to do.
However, I think it's wierd that lodash picks up the ${} syntax at all, so I also created a followup #48.

return compiledTemplateFn({ define })

if (define === undefined) {
return template
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe I'm wrong here, but I feel like this should be:

Suggested change
return template
return template || defaultHtmlTemplate

Copy link
Contributor Author

@denis-ok denis-ok Aug 6, 2023

Choose a reason for hiding this comment

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

You are right, good catch, thanks for checking!

Fixed slightly differently:
5fffc45

@denis-ok
Copy link
Contributor Author

denis-ok commented Aug 6, 2023

Thanks for this contribution. :)
I think skipping lodash _.template if define is not set is a reasonable thing to do.
However, I think it's wierd that lodash picks up the ${} syntax at all, so I also created a followup #48.

Good find! Yeah, it looks like a nice possible option to restrict a syntax for replacement.

@craftamap craftamap merged commit 5edeeab into craftamap:master Dec 20, 2023
2 checks passed
@craftamap
Copy link
Owner

Sorry this took a while:)

craftamap added a commit that referenced this pull request Aug 24, 2024
…ates

This patch disables using ${} in the html templates. This is mostly
to avoid conflicts with the provided template that might use ${} in
inline javascript natively.

This patch also reverts the changes of #46.
lodash.template is now always called. this is useful for when you want
to use `process.env`.

Therefore, this change is breaking, as it removes ${}, but also always
calls lodash again.

If this ever will be a problem, we can also add a configuration option
to enable ${} manually again.
craftamap added a commit that referenced this pull request Aug 27, 2024
…ates

This patch disables using ${} in the html templates. This is mostly
to avoid conflicts with the provided template that might use ${} in
inline javascript natively.

This patch also reverts the changes of #46.
lodash.template is now always called. this is useful for when you want
to use `process.env`.

Therefore, this change is breaking, as it removes ${}, but also always
calls lodash again.

If this ever will be a problem, we can also add a configuration option
to enable ${} manually again.
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.

2 participants