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

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

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

codyml
Copy link
Member

@codyml codyml commented Oct 12, 2022

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:

Screen Shot 2022-10-11 at 8 59 13 PM

Screen Shot 2022-10-11 at 8 59 24 PM

After:

Screen Shot 2022-10-11 at 9 00 27 PM

Screen Shot 2022-10-11 at 9 00 35 PM

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
Copy link

codecov bot commented Oct 12, 2022

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
Copy link
Member

zhaoyongjie commented Oct 12, 2022

@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
Copy link
Member

@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
Copy link
Member

rusackas commented Oct 12, 2022

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
Copy link
Member Author

codyml commented Oct 12, 2022

@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
Copy link
Member

rusackas commented Oct 13, 2022

@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!

@rusackas rusackas merged commit d5b4bde into apache:master Oct 13, 2022
@zhaoyongjie
Copy link
Member

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
Copy link
Member

🏷️ preset:2022.41

sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Oct 14, 2022
@lilykuang lilykuang added the v2.0 label Feb 23, 2023
lilykuang pushed a commit that referenced this pull request Feb 25, 2023
…d/runtime warnings/errors (#21779)

(cherry picked from commit d5b4bde)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants