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

Dynamic import for zxcvbn library #30104

Merged
merged 1 commit into from
Oct 25, 2022

Conversation

Oksydan
Copy link
Contributor

@Oksydan Oksydan commented Oct 23, 2022

Questions Answers
Branch? 8.0.x-rc
Description? zxcvbn library is rly huge and shouldn't be loaded in core.js. We are providing new filed to prestashop global variable core_js_public_path. It is being used for webpack publicPath on the fly to avoid problem when ruing prestashop inside catalog (domain.com/directory/). For now prestashop.checkPasswordScore is asynchronous and it's returning a promise. zxcvbn library is being loaded dynamically on calling prestashop.checkPasswordScore function. I will provide PR to classic and hummingbird.
Type? improvement
Category? FO
BC breaks? Yes and no. Since prestashop 8.0.0 isn't released yet but it will be if we don't implement it now.
Deprecations? no
Fixed ticket? Fixes #30088
How to test? 1. Build core.js assets (there should new file with suffix -chunk.js.
2. If testing with classic-theme PrestaShop/classic-theme#67 apply changes from this PR.
3. Go to FO register page.
4. Type inside password field, (you can open network tab in browser, there should be js file loaded on password check).
5. Password policy should be working as before.
Possible impacts? Check if it's working for prestashop installation with different physicial_uri

Before:
core.js size: 940kB

After:
core.js size: 144kB

@yanmakouf
Copy link
Contributor

@NeOMakinG 🦸‍♂️ 🚀 🔎

@Hlavtox
Copy link
Contributor

Hlavtox commented Oct 23, 2022

Remember that we want to load core js optionally - #29995 😎

Copy link

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

We definitely need this inside the 8.0 release or it will be a big blocker for the adoption

@Oksydan you're a monster btw

@@ -22,6 +22,8 @@
* @copyright Since 2007 PrestaShop SA and Contributors
* @license https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
*/
__webpack_public_path__ = window.prestashop.core_js_public_path;
Copy link
Contributor

@jolelievre jolelievre Oct 24, 2022

Choose a reason for hiding this comment

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

Not sure I get the purpose of this one? It allows using const zxcvbn = (await import('zxcvbn')).default; and import from chunks right?

Copy link
Contributor Author

@Oksydan Oksydan Oct 24, 2022

Choose a reason for hiding this comment

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

Hi @jolelievre,
Webpack need publicPath to be set so it can find files that have to be fetched example publicPath = "/themes/" - domain.com/themes/chunkfilename.
We can set publicPath in webpack configuration to just /themes/ and that should work in 99% but if store physical_uri is different than / for example /store/ (domain.com/store/) publicPath will be /store/themes/. That's why we need that variable to be set on the fly.

@khouloudbelguith khouloudbelguith self-assigned this Oct 24, 2022
@jolelievre jolelievre added the Waiting for QA Status: action required, waiting for test feedback label Oct 24, 2022
Copy link
Contributor

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hello @nicosomb,

I checked those cases
Case1
image
Physical_uri === /Presta+Shop/ => OK

  • Install > Information step
  • FO > Create an account
  • FO > Edit information
  • FO > Create an account for a guest
  • FO > Checkout > Create an account

Case2
image
Physical_uri === /PrestaShop/ => OK

  • Install > Information step => OK
  • FO > Create an account => OK
  • FO > Edit information => OK
  • FO > Create an account for a guest => OK
  • FO > Checkout > Create an account => OK

Case3
image
Physical_uri === / => OK

  • Install > Information step => OK
  • FO > Create an account => OK
  • FO > Edit information => OK
  • FO > Create an account for a guest => OK
  • FO > Checkout > Create an account => OK

I attached screen records

Check.1.-.base.uri._.mp4

Case4
image
Physical_uri === /test/80x/ => NOK
Install > Information step: OK
BO > Create a customer => OK
BO > Edit an employee password => OK
FO > Create an account => NOK
FO > Edit information => NOK
FO > Checkout > Create an account => NOK

The info messages are not displayed when typing the password
I attached a screen record

base.uri._test_80x_.NOK.mp4

I tried with 8.0.0RC1 => OK

8.0.0RC1.with.PR.ok.mp4

Thanks!

@khouloudbelguith khouloudbelguith added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Oct 24, 2022
@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 24, 2022

Hi @khouloudbelguith,
In not working case your network tab is showing that file has been fetched properly. There is a js error in your console ever time you type in password field. Did you apply changes to classic when you were testing that case 4? Are you able to show me that console with errors?
Thanks

Copy link
Contributor

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hi @Oksydan,

Thanks for your feedback.
After some rechecks, it is ok ✔️
I checked with this
image

Also, automatic tests => OK ✔️
I checked when multistore enabled / I checked friendly URL enabled & disabled => OK ✔️

final_check.mp4

Also, I checked the size of core.js => ok
image

Before, it was Nok
image

Thanks!

@khouloudbelguith khouloudbelguith added QA ✔️ Status: check done, code approved and removed Waiting for author Status: action required, waiting for author feedback labels Oct 25, 2022
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@kpodemski kpodemski added this to the 8.0.0 milestone Oct 25, 2022
@kpodemski kpodemski merged commit e96148b into PrestaShop:8.0.0-rc Oct 25, 2022
@kpodemski
Copy link
Contributor

Thank you @Oksydan @khouloudbelguith

@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 25, 2022

@khouloudbelguith good job. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants