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

Fixes #28448 - Use Intl npm modules from vendor #7233

Closed
wants to merge 1 commit into from

Conversation

sharvit
Copy link
Contributor

@sharvit sharvit commented Dec 6, 2019

Load jed, intl and react-intl from @theforeman/vendor.
Support async import.

Work with: theforeman/foreman-js#81

@Ron-Lavi
Copy link
Member

Ron-Lavi commented Dec 9, 2019

tests are failing on travis seem related,
and locally I am getting the error in I18n.test.js:

Summary of all failing tests
 FAIL  webpack/assets/javascripts/react_app/common/I18n.test.js
  ● Test suite failed to run

SyntaxError: /home/rlavi/projects/foreman-js/packages/vendor-core/lib/customModules/vendorIntl.js: Support for the experimental syntax 'dynamicImport' isn't currently enabled (2:3):

      1 | export const loadVendorIntl = () =>
    > 2 |   import(/* webpackChunkName: "intl" */ 'intl');
        |   ^
      3 | 
      4 | export const loadVendorIntlLocale = locale =>
      5 |   import(

    Add @babel/plugin-syntax-dynamic-import (https://git.io/vb4Sv) to the 'plugins' section of your Babel config to enable parsing.

      at Parser.raise (node_modules/@babel/parser/src/parser/location.js:41:63)
      at Parser.expectPlugin (node_modules/@babel/parser/src/parser/util.js:165:18)
      at Parser.parseExprAtom (node_modules/@babel/parser/src/parser/expression.js:952:14)
      at Parser.parseExprSubscripts (node_modules/@babel/parser/src/parser/expression.js:553:23)
      at Parser.parseMaybeUnary (node_modules/@babel/parser/src/parser/expression.js:533:21)
      at Parser.parseExprOps (node_modules/@babel/parser/src/parser/expression.js:301:23)
      at Parser.parseMaybeConditional (node_modules/@babel/parser/src/parser/expression.js:256:23)
      at Parser.parseMaybeAssign (node_modules/@babel/parser/src/parser/expression.js:186:21)
      at Parser.parseFunctionBody (node_modules/@babel/parser/src/parser/expression.js:1959:24)
      at Parser.parseArrowExpression (node_modules/@babel/parser/src/parser/expression.js:1898:10)

@xprazak2
Copy link
Contributor

xprazak2 commented Dec 9, 2019

Works, but I get errors in console:

missing-locale

@sharvit
Copy link
Contributor Author

sharvit commented Dec 9, 2019

tests are failing on travis seem related,
and locally I am getting the error in I18n.test.js:

Summary of all failing tests
 FAIL  webpack/assets/javascripts/react_app/common/I18n.test.js
  ● Test suite failed to run

Tests fail in Travis because this should work with theforeman/foreman-js#81 and Travis uses an older version from npm (what we have in the package.json).
In order for it to work, it should be used together with the foreman-js PR.
Once we will merge theforeman/foreman-js#81 and will update the package.json to use the new version, Travis should work.

About your Add @babel/plugin-syntax-dynamic-import error, I could not reproduce it but it looks like it didn't load your env/babel configs. Looks like it is related to your environment somehow.
Are you sure you did?:

cd foreman-js
npm run clean
npm i
cd ../foreman
rm -rf node_modules package-lock.json
npm i
npm run foreman-js:link

@Ron-Lavi
Copy link
Member

thanks @sharvit, I did the following and that specific test keeps failing :(

@coveralls
Copy link

coveralls commented Dec 13, 2019

Coverage Status

Coverage decreased (-0.01%) to 74.907% when pulling 12ad4d8 on sharvit:feat/vendor-intl into ee84597 on theforeman:develop.

package.json Outdated
"intl": "~1.2.5",
"jed": "^1.1.1",
"react-intl": "^2.8.0"
"@theforeman/vendor": "3.8.0-intl.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deployed this version to npm so it will be easier to test.
It should still be tested against theforeman/foreman-js#81 locally to make sure both work well.

@sharvit sharvit changed the title Fixes #28448 - Use Intl npm modules from vendor WIP: Fixes #28448 - Use Intl npm modules from vendor Dec 15, 2019
@sharvit sharvit added the WIP label Dec 15, 2019
@sharvit
Copy link
Contributor Author

sharvit commented Dec 15, 2019

Thanks for the reviews.

@LaViro - I have fixed the testing issue and looks like Travis is happy as well. Can you verify please?

@xprazak2 - still investigating about the local change

@Ron-Lavi
Copy link
Member

Awesome work @sharvit!

I have great news and bad news:

  • Great news - works well in Foreman, found no regression. tests and linting are working.
  • Bad news - on each plugins page, the page fails with the following error:
MountingService.js:89 Uncaught DOMException: Failed to execute 'define' on 'CustomElementRegistry': the name "react-component" has already been used with this registry
    at Object.<anonymous> (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:32833:23)
    at __webpack_require__ (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:679:30)
    at fn (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:89:20)
    at Object.<anonymous> (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:68038:94)
    at __webpack_require__ (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:679:30)
    at fn (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:89:20)
    at Object.<anonymous> (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:44342:65)
    at __webpack_require__ (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:679:30)
    at fn (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:89:20)
    at Object.<anonymous> (https://centos7-luna-devel.rlavi.example.com:3808/webpack/katello.js:44331:18)

@sharvit
Copy link
Contributor Author

sharvit commented Jan 1, 2020

Thanks @xprazak2 I fixed the issue with changing the locale.

@sharvit
Copy link
Contributor Author

sharvit commented Jan 1, 2020

@LaViro I fixed the issue you mention and I have Great News 😄

The Issue
We have technical debt in the javascript-stack where the react_app folder get copied to each plugin 👎
See: https://projects.theforeman.org/issues/27195
Somehow everything worked fine but once I removed the old vendor and the CommonChunkPlugin, the issue appear because now it executes each plugin's react_app.

The solution
Using webpack CommonChunkPlugin to build the react_app into foreman-react.js and reuse it across plugins.
This fixes the technical debt and now everything works, even better.

@sharvit sharvit changed the title WIP: Fixes #28448 - Use Intl npm modules from vendor Fixes #28448 - Use Intl npm modules from vendor Jan 1, 2020
@sharvit sharvit removed the WIP label Jan 1, 2020
@sharvit
Copy link
Contributor Author

sharvit commented Jan 1, 2020

Notice this change will break nightlies and we will need to rebuild all the plugins after merging this PR

@tbrisker
Copy link
Member

@sharvit is this PR still valid or should we close it?

@tbrisker
Copy link
Member

Thanks @sharvit !

@tbrisker tbrisker closed this Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants