OpenEnergyDashboard / OED

Open Energy Dashboard (OED)
Mozilla Public License 2.0
82 stars 307 forks source link

fix: Use static plotly.js imports to make Vite happy #1060

Closed hyperupcall closed 12 months ago

hyperupcall commented 1 year ago

Description

Vite will not play nice with these dynamic requires for some reason. To make Vite happy, convert the imports to static imports. Also change them to ESM-imports for consistency.

Related to #879

Type of change

Checklist

Limitations

N/A

huss commented 1 year ago

@hyperupcall Thanks for working on this. It all looked fine but testing found the language is not changing in Poltly. This is what I get with the PR when I set the language to Spanish: Screenshot 2023-11-07 122206 and this is from the development branch: Screenshot 2023-11-07 121746 Notice Jan becomes Ene as expected. I tried this with PR #1059 and it works the same as development so this change seems the likely source. I have not had time to look into the reason. If you can look at this or have any ideas then I welcome that.

hyperupcall commented 12 months ago

Looking at the source code local file, it seems they use module.exports to create a "default import export".

It seems Webpack complies with the JavaScript Specification and CommonJS specification in the following way:

But, Vite should do this too. And it should be okay to require a CommonJS file inline. It could be possible the error was something to do with tree-shacking, but I don't remember. One thing to note is that plotly.js has a webpack field in it's package.json. That could explain the discrepancy: Vite and Webpack are reading different files (Webpack, packageJson.webpack and Vite, packageJson.main).

hyperupcall commented 12 months ago

I think we should close this issue bcause it works with Webpack, and we're using Webpack right now. I originally made the change because Vite ~v3.3 previously errored and was unhappy. Now, Vite has version v4.0 (and is even faster). It may be possible that this issue has been fixed or resolved in Vite. In any case, I think it might be better to resolve this once Vite is integrated, so we have a chance to test this with Vite. What do you think?

huss commented 12 months ago

I think we should close this issue bcause it works with Webpack, and we're using Webpack right now. I originally made the change because Vite ~v3.3 previously errored and was unhappy. Now, Vite has version v4.0 (and is even faster). It may be possible that this issue has been fixed or resolved in Vite. In any case, I think it might be better to resolve this once Vite is integrated, so we have a chance to test this with Vite. What do you think?

I'm okay with closing this pull request. I'm hoping we can get Vite incorporated into OED in the foreseeable future. Thanks for looking after this.

hyperupcall commented 12 months ago

I'm okay with closing this pull request. I'm hoping we can get Vite incorporated into OED in the foreseeable future. Thanks for looking after this.

Me too! Since #1086 was merged, I can focus my efforts on that.