fix(plugin-chart-handlebars): Update webpack/babel config to fix build/runtime warnings/errors

[codyml]

SUMMARY

This PR is an attempt to fix the Handlebars viz plugin, which currently raises both build and runtime errors, by updating Webpack and Babel config.

Build warning

Webpack would emit the following warning several times during build: require.extensions is not supported by webpack. Use a loader instead.. The source listed was various lines in ./node_modules/handlebars/lib/index.js. According to the Handlebars GH project, this is due to the package not correctly indicating which build is client-side-compatible. A fix was merged, but it hasn't made it into an NPM-published release. A workaround was recommended by this comment and is included in this PR. This issue doesn't seem to be related to the runtime error and this fix just clears the build warning.

Runtime error

When rendering a Handlebars chart in Explore, the following error appears: ReferenceError: exports is not defined. An inspection of the console shows that the culprit is the just-handlebars-helpers package's references to exports, as is standard in the CJS module system. After some digging, I found this comment that makes me think the problem is a CJS/ESM incompatibility: our Babel setup assumes all JS is ESM-style modules and therefore doesn't transpile CJS-style require and module.exports statements into import/export statements, and Webpack assumes Babel output is ESM-style modules and doesn't provide global require/module.exports that you'd find in a CJS environment, causing this error (just-handlebars-helpers seems to be CJS-only). As suggested by the comment, I changed sourceType to unambiguous so it stops assuming modules are ESM-style and instead makes a guess based on the presence of require/module.exports. This seems to fix the issue, though I don't have a great sense of what else this might affect.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

• Run webpack and ensure the aforementioned build warnings are gone.
• Try creating a Handlebars chart and ensure the runtime error doesn't show up.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #21779 (d5b5b40) into master (406e44b) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21779      +/-   ##
==========================================
- Coverage   66.88%   66.85%   -0.03%     
==========================================
  Files        1800     1803       +3     
  Lines       68967    68998      +31     
  Branches     7339     7350      +11     
==========================================
+ Hits        46128    46131       +3     
- Misses      20943    20972      +29     
+ Partials     1896     1895       -1     

Flag	Coverage Δ
javascript	53.19% <ø> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ontrols/DateFilterControl/utils/dateFilterUtils.ts	39.39% <0.00%> (-60.61%)	⬇️
...nd/src/dashboard/components/gridComponents/Row.jsx	51.21% <0.00%> (-23.79%)	⬇️
.../explore/exploreUtils/getParsedExploreURLParams.ts	83.33% <0.00%> (-8.10%)	⬇️
...nts/controls/DateFilterControl/DateFilterLabel.tsx	35.71% <0.00%> (-5.70%)	⬇️
...ontrols/DndColumnSelectControl/DndFilterSelect.tsx	44.03% <0.00%> (-1.01%)	⬇️
superset-frontend/src/components/Chart/Chart.jsx	51.66% <0.00%> (-0.88%)	⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx	53.44% <0.00%> (-0.79%)	⬇️
superset-frontend/src/explore/constants.ts	100.00% <0.00%> (ø)
...et-frontend/src/explore/reducers/exploreReducer.js	37.50% <0.00%> (ø)
...ntend/plugins/plugin-chart-table/src/buildQuery.ts	56.25% <0.00%> (ø)
... and 38 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[zhaoyongjie]

@codyml When I looked into the dependency that Just Handlebars Helpers, it said that A lightweight package with common Handlebars helpers. from the npm registry description. It just provided some shortcuts for the Handlebars, so it's not a core function for the Handlebar. This plugin was introduced from PR.

IMO, I would rather remove the Helper than change the global Bable setting to fit a plugin's configuration.

What do you think about this? @evans @villebro

[villebro]

@codyml When I looked into the dependency that Just Handlebars Helpers, it said that A lightweight package with common Handlebars helpers. from the npm registry description. It just provided some shortcuts for the Handlebars, so it's not a core function for the Handlebar. This plugin was introduced from PR.

IMO, I would rather remove the Helper than change the global Bable setting to fit a plugin's configuration.

What do you think about this? @evans @villebro

I believe there's actually an open PR out there to do exactly this: #21143 (the discussion there is pretty similar to what you're proposing). I think this comes down to whether or not we feel maintaining these helpers ourselves is something we want to do or not. Normally I'd say let's just use the npm package, but if using the upstream package requires us to do hacky workarounds, then I'm not so sure.

[rusackas]

Heya @zhaoyongjie / @villebro

It's true, I don't like the idea of copying over all the internals of that helper module into our codebase for a couple of reasons:

1. It feels "gross" from a copyright/licensing/plagiarism perspective
2. We would lose the benefit of any updates/expansions that the other project takes on.

So, I'd like to use this just-handlebars-helpers module if possible. I'm also open to competitors, but nothing obvious turns up except for this one which seems unmaintained.

I'll open up an Issue on the just-handlebars-helpers repo to see if they would be willing to create an EJS version. Otherwise, if we can change the babel config for a specific file path, that'd be a little more surgical than this PR currently is.

However, the big question I have is: do we see any realistic risk or downslides to this change in babel config? Maybe we can approve/merge this until the day that just-handlebars-helpers adds EJS (maybe we write a PR there!) at which point we can lock this back down.

Total side note: Apologies to Evans, who gets pinged by this project all the time, and is not me ;)

[codyml]

@zhaoyongjie @villebro @rusackas If it helps, I just pushed a diff that overrides the Babel sourceType for just the package in question rather than the whole project!

[rusackas]

@zhaoyongjie @villebro @rusackas If it helps, I just pushed a diff that overrides the Babel sourceType for just the package in question rather than the whole project!

Thanks! I spent a few minutes looking for a way to do that, but didn't see one! This looks good to me!

[zhaoyongjie]

Total side note: Apologies to Evans, who gets pinged by this project all the time, and is not me ;)

I really wanted to say sorry to Evan, and buy him a drink.

[sadpandajoe]

🏷️ preset:2022.41