fix(plugin-chart-handlebars): Fixed issue with Helpers

[sinhashubham95]

SUMMARY

The library just-handlebar-helpers previously used as helpers for handlebars requires a lot of dependencies like moment, etc, which need not be defined necessarily. Also, it expects some globals which are breaking on window machines. Switched to having the necessary helpers in the plugin itself.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

Render any chart using plugin-chart-handlerbars on any dataset. It should render without any error.

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

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️
We hope to see you in our Slack community too!

[sinhashubham95]

@kgabryje @lyndsiWilliams can you please review?

[github-actions]

Storybook has completed and can be viewed at

[sinhashubham95]

@zhaoyongjie @stephenLYZ @michael-s-molina can you please check this pr? It would be very helpful, it has been open since long.

[github-actions]

Storybook has completed and can be viewed at

[sinhashubham95]

@AAfghahi can you please check this pull request?

[sinhashubham95]

@hughhhh @zhaoyongjie @stephenLYZ @michael-s-molina can you please check this PR?

[sinhashubham95]

@villebro can you please check this?

[codecov]

Codecov Report

Merging #21143 (55b6341) into master (7e2e8b8) will increase coverage by 0.04%.
The diff coverage is 65.71%.

@@            Coverage Diff             @@
##           master   #21143      +/-   ##
==========================================
+ Coverage   66.57%   66.61%   +0.04%     
==========================================
  Files        1793     1796       +3     
  Lines       68493    68339     -154     
  Branches     7275     7312      +37     
==========================================
- Hits        45596    45521      -75     
+ Misses      21035    20948      -87     
- Partials     1862     1870       +8     

Flag	Coverage Δ
javascript	52.87% <65.71%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
...ars/src/components/Handlebars/HandlebarsViewer.tsx	0.00% <0.00%> (ø)
...lugins/plugin-chart-handlebars/src/helpers/html.ts	33.33% <33.33%> (ø)
...ugins/plugin-chart-handlebars/src/helpers/utils.ts	42.85% <42.85%> (ø)
...lugin-chart-handlebars/src/helpers/conditionals.ts	59.52% <59.52%> (ø)
...ins/plugin-chart-handlebars/src/helpers/strings.ts	87.87% <87.87%> (ø)
...lugins/plugin-chart-handlebars/src/helpers/math.ts	100.00% <100.00%> (ø)
superset/datasets/commands/delete.py	93.18% <0.00%> (-3.97%)	⬇️
superset/databases/commands/validate.py	74.13% <0.00%> (-3.45%)	⬇️
superset/explore/commands/get.py	89.65% <0.00%> (-2.30%)	⬇️
superset/datasets/commands/create.py	98.03% <0.00%> (-1.97%)	⬇️
... and 48 more

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

[villebro]

@villebro can you please check this?

@sinhashubham95 sure thing, will check in a few hours!

[villebro]

@sinhashubham95 just so I understand this PR correctly, are we essentially copying over the functions from just-handlebar-helpers? While it may make it possible to drop some unnecessary requirements, we're already pulling in moment as it's needed elsewhere, so dropping just-handlebars-helpers won't remove that dependency. Also, I notice the main lock file hasn't been updated, so we'd at least need to rebuild the main package-lock.json file which appears to still be referencing just-handlebars-helpers.

[sinhashubham95]

@villebro I have updated the package lock file.

The idea behind updating the package lock file is-

1. Just handlebars helpers usage was causing an issue saying exports not defined which is making the current handlebars plugin unusable in Superset.
2. Also, the plugin handlebars can be lighter by removing this dependency and adding the methods required directly. Here we have added a subset of the methods which is available in the helpers package.

[villebro]

@sinhashubham95 thanks for the clarification. Did I understand correctly that you're getting the export error when running on Windows? While I would like to help support a more diverse se of OSes, currently Superset does not officially support running the application on Windows. So if this is the main reason for removing just-handlebars-helpers then we need to consider the following tradeoffs:

• removing dependency on just-handlebars-helpers reduces dependencies, but at the same time we loose any fixes from the upstream package by copying over the current subset of functions
• Trying to support Windows may introduce other changes that can be a maintenance burden on the project. AFAIK none of the core committers are running Superset on anything but MacOS and Linux, so committing to support Windows in the absence of any dedicated core developers will likely be difficult. Again, it's great if we can support as wide an array of OSes as possible, but any additionally supported platform, be it implicit or explicit, will always introduce additional maintenance burden.

Out of curiosity, would running on a Linux server be an option? This is currently the recommended setup.

[sinhashubham95]

@villebro I am facing the issue with both MacOS and Linux. I understand this part that removing the dependency will avoid any fixes to be incorporated automatically in Superset, but this is what I believe around this, the helpers here are very straightforward, and adding any extra dependency for such functions won't add any benefit but just bloat up our core Superset. Using libraries like moment, etc. makes a lot of sense for the vast set of functionalities and the community support it gets.

[villebro]

I am facing the issue with both MacOS and Linux.

Ok, if you're running into trouble on MacOS then this needs additional attention. Can you check what versions of Node and npm you're using? My output below:

$ node -v && npm -v
v16.9.1
7.21.1

I understand the argument about this functionality being simple and I agree - however, if we want to incorporate this logic into Superset, I would strongly recommend adding full unit tests to ensure we have full test coverage for each supported function.

Ping @rusackas , any thoughts on this?

[sinhashubham95]

@villebro I am using node v16.16.0 and npm v8.11.0. Also, I have already added test cases around the functions I added.

[villebro]

I am using node v16.16.0 and npm v8.11.0. Also, I have already added test cases around the functions I added.

Oh my bad - the tests look great, thanks for adding those 👍 I'm leaning towards your proposed solution, thanks for all the iterations here. Let's also wait if anyone else has any feedback before proceeding.

[sinhashubham95]

@villebro thanks a lot for these iterations, it's just a healthy discussion and learning for me. Sure, lemme know whenever we can merge this, it has kind of been open for long. Another update, after this change I tested both on MacOS and Linux and it works perfectly fine.

[sinhashubham95]

@villebro are we good to merge this?

[rusackas]

I really, really appreciate all the work happening in this PR, but I'm a little nervous about this one... it seems like we're taking on much of the functionality of a whole library in terms of stability/security/maintenance. And not the whole library, including time and currency formatters, for example.

I don't think the optimization of removing moment is all that pertinent to the project, but the exports not defined one might be. I haven't noticed this error on Mac, and I don't have a Windows machine to test. Has there been much investigation into testing/resolving this error, other than the approach herein?

Last and maybe least (if I'm over-thinking this), but this seems to be a copy/paste of the just-handlebars-helpers codebase, not an original implementation and would be subject to the copyright of the original library. It's MIT licensed, so we would need to include the author's copyright, and I'm not sure how including MIT licensed code directly within an Apache-licensed file works. ¯\_(ツ)_/¯

[rusackas]

@sinhashubham95 @villebro just checking in on this... wondering if I'm off base in my prior assessment.

The build issue was resolved quite some time ago, and I don't think the moment dependency is a big deal, personally.

Is anyone still clamoring to get this lib copied/pasted into our codebase, and willing to maintain it? If not, I think we should probably forego this change and close the PR.

[rusackas]

Closing, but still happy to revisit this discussion if/when needed