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

Rename constants/Language module to the right suffix, and other actions to make ESM bundlers happy. #6130

Merged
merged 12 commits into from
Jun 28, 2024

Conversation

sneridagh
Copy link
Member

@sneridagh sneridagh commented Jun 27, 2024

  • Add proper dependencies to volto-slate

📚 Documentation preview 📚: https://volto--6130.org.readthedocs.build/

Copy link

netlify bot commented Jun 27, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit da7276d
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/667e628f47aac300089a8dfc

@sneridagh
Copy link
Member Author

@stevepiercy a bunch of my crappy english upgrade guide paragraphs.

Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sneridagh I think this change is fine, waiting for @stevepiercy magic for docs check

docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
packages/volto-slate/news/6130.internal Outdated Show resolved Hide resolved
packages/volto/news/6130.internal Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

@stevepiercy a bunch of my crappy english upgrade guide paragraphs.

They may be crappy, but they are my favorite, just like I'm my parents' favorite son. 😉

sneridagh and others added 2 commits June 28, 2024 09:11
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@sneridagh sneridagh changed the title Fix remaining module.exports non ESM in code Rename constants/Language module to the right suffix, and other actions to make ESM bundlers happy. Jun 28, 2024
@sneridagh sneridagh merged commit 502c0f7 into main Jun 28, 2024
70 of 71 checks passed
@sneridagh sneridagh deleted the fixrandommoduleexports branch June 28, 2024 08:13
@ichim-david
Copy link
Member

@sneridagh the only one that needs to have the module.export definition is razzle.config which makes use of this file as seen in https://github.com/plone/volto/blob/main/packages/volto/razzle.config.js#L23

Looking at your vite WIP https://github.com/plone/volto/pull/6050/files#diff-54808bcea9a0f79709ba2a13c85f0964d7828cb604f21a819342384570bee024
I see that you converted the Language.js to a ESM module since it's used by both server.jsx and PersonalPreferences.jsx and in the browser it says that the file needs to have an export default if it's not provided.

Right now with this logic the language selector no longer displays the languages options because
import languages from "languages.cjs"
is resolved to "http://localhost:3001/static/media/Languages.ea358519.cjs"

Rather than trying to convert somehow the CJS to ESM, bring in extra babel plugins and given the need of vite for ESM module I propose to Revert your change as @wesleybl suggests

Later we can better handle languages.js
One solution for later would be something like:

  1. Make Languages.js an ESM module with export default languages.
  2. Figure out how to get the languages object importable within razzle.config
    (A stupid simple way would be to create Languages.cjs that duplicates the logic of Languages.js) and we know to remove it when we no longer use razzle.

@wesleybl
Copy link
Member

wesleybl commented Aug 5, 2024

@sneridagh @ichim-david if the file is renamed to languages.js, it will work in server.js, even if it contains module.exports. So, if the problem you encountered was due to the export method, just renaming the file will work in both situations.

@ichim-david
Copy link
Member

ichim-david commented Aug 5, 2024

@sneridagh @ichim-david if the file is renamed to languages.js, it will work in server.js, even if it contains module.exports. So, if the problem you encountered was due to the export method, just renaming the file will work in both situations.

@wesleybl yes it's correct that if we have language.js our babel, webpack and razzle config knows how to modify that module.exports to be correctly bundled even when it's used by razzle.config which takes this common js module format
and the imports (esm) way from server.jsx and personaltools.jsx.
The problem is that vite doesn't know how to handle that, it doesn't recognize the module keyword and if you look at @sneridagh vite WIP he is actually using export default in Languages.js
https://github.com/plone/volto/pull/6050/files#diff-54808bcea9a0f79709ba2a13c85f0964d7828cb604f21a819342384570bee024

To me this makes it clear that this pull request did the opposite of what should have been done, to turn Language.js into an ESM module and only in razzle.config where it's set to require('languages') to have had some way of importing that object.
This is because if you were to try this vite branch and replace export default with module.exports like we had before this work in Languages.js you will see it complains in the browser devtools that there is no export in Languages.js.

Unfortunately at that time I didn't do a good enough job in understanding and testing the impact of this pull request and now we have this issue at hand. EDIT: we need to have cypress tests for the personal properties languages so that we catch potential issues in the future.

We could revert this work (not with git revert the whole pull request but doing another pull request and changing back the language.js rename and then removing the upgrade notice) and worry about getting Language.js into a state that is accepted by vite later but since Victor in on a holiday we need input from @davisagli on what he proposes as well, maybe he has a better idea that what I suggested here #6130 (comment)

@wesleybl
Copy link
Member

wesleybl commented Aug 5, 2024

The problem is that vite doesn't know how to handle that, it doesn't recognize the module keyword and if you look at @sneridagh vite WIP he is actually using export default in Languages.js

@ichim-david today, Languages.cjs uses the keyword module:

Does this mean that we still have a problem in Vite, even after this PR? Do you know how I can simulate the problem in Vite?

@ichim-david
Copy link
Member

The problem is that vite doesn't know how to handle that, it doesn't recognize the module keyword and if you look at @sneridagh vite WIP he is actually using export default in Languages.js

@ichim-david today, Languages.cjs uses the keyword module:

Does this mean that we still have a problem in Vite, even after this PR? Do you know how I can simulate the problem in Vite?

@wesleybl I wrote in my reply above checkout the razzle no build vite wip pull request. Modify the language.js to use module.export instead of export default and start vite with pnpm dev. Load localhost:3000 and you will see you don't get the front page loaded due to error in language file

@davisagli
Copy link
Member

@ichim-david @wesleybl Instead of changing languages.cjs, I suggest we fix loading of .cjs files. This seems to work: #6237

sneridagh added a commit that referenced this pull request Sep 14, 2024
* main:
  Automatically add the PLIP to the PLIP project board (#6134)
  Fix link to renamed `src/constants/Languages.cjs` (#6135)
  Release 18.0.0-alpha.39
  Release generate-volto 9.0.0-alpha.17
  rename test-setup-config.js to test-setup-config.jsx (#6133)
  Cleanup Image widget and pass down `onSelectItem` prop if any (#6132)
  Release 18.0.0-alpha.38
  Release @plone/slate 18.0.0-alpha.15
  Rename `constants/Language` module to the right suffix, and other actions to make ESM bundlers happy. (#6130)
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.

5 participants