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 cookie import #373

Merged
merged 4 commits into from
Sep 16, 2021
Merged

Fix cookie import #373

merged 4 commits into from
Sep 16, 2021

Conversation

jblz
Copy link
Contributor

@jblz jblz commented Sep 11, 2021

Description

When checking out #366, I saw a warning:

WARNING in ./src/js/lib/personalization.js 7:9-18
export 'get' (imported as 'getCookie') was not found in 'js-cookie' (possible exports: default)
 @ ./src/js/widgets/recommended/index.js 15:0-69 27:28-52

...even though our tests still passed... 😕

This syntax makes the warning go away.

Motivation and Context

I'm not sure why, but it seems like webpack 5 (or the WP wrapper) didn't work w/ the existing style import. The "new" way follows the directions on the tin, so this looks like the way to go.

How Has This Been Tested?

See #364

Automated testing

npm run test

Manual testing

  • Run this branch on a site with API access
  • Configure the widget to display (in a sidebar, footer, etc.)
  • Browse to a page with the widget on it
    • Confirm that personalized results are requested / received
    • Namely, you should see a populated uuid query argument in the call to https://api.parsely.com/v2/related as per this line.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)

@jblz jblz requested a review from a team as a code owner September 11, 2021 06:26
@jblz jblz added this to the 2.5.2 milestone Sep 11, 2021
@jblz jblz mentioned this pull request Sep 11, 2021
10 tasks
Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

How can we improve our unit tests so that the old way would fail?

Or is this just a warning but the import code still worked anyway?

@pauarge
Copy link
Contributor

pauarge commented Sep 15, 2021

@GaryJones Been taking a look at your suggestion. js.cookie.mjs exports by default a function (api) that creates an object with the get property. That is why the code would work fine but trigger the warning. By importing it the way Jeff does in this PR, we are importing the default export hence no warning.

I'm haven't found a solution for improving the unit tests, so I guess we're good by merging the code as is.

@pauarge pauarge merged commit 82fb0ca into develop Sep 16, 2021
@pauarge pauarge deleted the fix-cookie-import branch September 16, 2021 08:03
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.

3 participants