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: Use static plotly.js imports to make Vite happy #1060

Conversation

hyperupcall
Copy link
Contributor

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

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

N/A

Vite didn't 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.
@huss
Copy link
Member

huss commented Nov 7, 2023

@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
Copy link
Contributor Author

hyperupcall commented Nov 20, 2023

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:

  • When using import, Webpack will bundle the code with the semantics of ECMAScript Modules. That is, the default import is specified with export default, so the bundled code "ignores" the module.exports because that's not a part of ESModules
    • Usually in these situations, the imported file contains a require, and crashes the program. Since there are no requires in locales/es.js, that is why there is a silent failure
  • When using require, Webpack will bundle the code with the semantics of CommonJS. That is, it will "look for" module.exports to find the default import (and exports.default, depending on the configuration)

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

hyperupcall commented Nov 20, 2023

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

huss commented Nov 21, 2023

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

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.

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