Move PriceSummaryFragment to peregrine #3007

larsroettig · 2021-02-10T21:32:00Z

Description

Moves PriceSummaryFragment to peregrine to allow partner to build headless exteriperiance without venia
This refactoring helps also Extension providers to not have query duplikation

Related Issue

Closes PWA-1461.

Acceptance

Test Cart
Test Checkout

Verification Stakeholders

@sirugh @revanth0212

Specification

Verification Steps

Add Product to cart
Go to cart page
Make checkout with braintree

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

Resolved issues:

resolves [chore/feature] Move PriceSummaryFragment to peregrine #3010: Move PriceSummaryFragment to peregrine

PWAStudioBot · 2021-02-10T21:42:30Z

	Messages
📖	Associated JIRA tickets: PWA-1461.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against de23661

… can read those extensions in peregrine

larsroettig · 2021-02-11T18:46:47Z

System for Test https://pr-3007.pwa-venia.com/checkout

sirugh · 2021-02-11T18:51:39Z

@magento create issue from PR

revanth0212

Thanks for the contribution @larsroettig. The code looks fine and works as expected. I only have 1 change request from you, please check the comment out and address it as required.

revanth0212 · 2021-02-16T16:10:33Z

jest.config.js

@@ -136,7 +139,7 @@ const testReactComponents = inPackage => ({
        '@magento/venia-drivers':
            '<rootDir>/packages/venia-ui/lib/drivers/index.js'
    },
-    moduleFileExtensions: ['ee.js', 'ce.js', 'js', 'json', 'jsx', 'node'],
+    moduleFileExtensions: [...defaults.moduleFileExtensions, 'ee.js', 'ce.js'],


Is there a reason you made this change?

I personally don't think this is a valid change. You changed the priority order of the file extension matching algorithm. Now ee.js and ce.js have the lowest priority and would never match against a file because .js has higher priority and will go through.

Also, we have this same file matching logic in webpack, if tomorrow's defaults.moduleFileExtensions changes but webpack matching is different, this will cause problems.

I made this change. Is moduleFileExtensions in order of priority? This is jest config, not webpack: https://jestjs.io/docs/en/webpack#configuring-jest-to-find-our-files

As far as I remember yes. For instance, if you have component.js and component.json in the same folder you try to import component since .js has higher priority, it will be picked over .json file. And we follow the same array convention in webpack configuration as well. It is beneficial if these 2 stay in sync. Webpack uses a similar import configuration while building which jest uses while testing.

You're right! We should find a way to sync these! We should export the module resolution array from some central location. @larsroettig if you can do that, would be awesome, otherwise we can just revert this change back to how it was before I modified it.

revanth0212 · 2021-02-16T16:11:49Z

jest.config.js

@@ -230,6 +233,12 @@ const jestConfig = {
        })),
        configureProject('pagebuilder', 'Pagebuilder', testReactComponents),
        configureProject('peregrine', 'Peregrine', inPackage => ({
+            // Make sure we can test extension files.
+            moduleFileExtensions: [


Same here, I would suggest using file extension array from develop.

I was suggesting to use ['ee.js', 'ce.js', 'js', 'json', 'jsx', 'node'] as the value of moduleFileExtensions.

And you'd prefer that over ['ee.js', 'ce.js', ...defaults.moduleFileExtensions] so we're explicit?

Probably easiest just to change this to Revanth's suggestion, @larsroettig

And you'd prefer that over ['ee.js', 'ce.js', ...defaults.moduleFileExtensions] so we're explicit?

I like the idea of maintaining these in a global location somewhere and importing them in the jest and webpack config files instead of relying on jest.moduleFileExtensions which can change in a future version.

I don't think we can use 1 array for both since the syntaxes they employ are different.

For instance,

Webpack config: ['.ee.js', '.ce.js', '.js', '.json', '.jsx', '.node']
Jest config: ['ee.js', 'ce.js', 'js', 'json', 'jsx', 'node']

Webpack needs . in front of the file extensions while jest doesn't.

The best solution at the moment would be to revert this change to what it was in develop and handle it in a different ticket if deemed necessary.

larsroettig · 2021-02-17T21:05:35Z

@revanth0212 i updated this PR

jest.config.js

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

revanth0212

Almost there. Some prettier changes needed. Please check.

larsroettig · 2021-02-19T16:36:01Z

@dpatil-magento and @revanth0212 can be procced this because i want finish my checkmo feedture

wip

bb4a278

magento-engcom-team added Partner: TechDivision partners-contribution labels Feb 10, 2021

m2-community-project bot added the Progress: ready for review label Feb 10, 2021

larsroettig changed the title ~~wip~~ Move PriceSummaryFragment to peregrine Feb 10, 2021

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Feb 10, 2021

larsroettig marked this pull request as draft February 10, 2021 21:46

Lars and others added 6 commits February 10, 2021 23:12

Move PriceSummaryFragment to peregrine

e8623d8

Fix unit tests

11cc258

Fix lint

f38e1b2

Merge branch 'develop' into refactoringPricesummaryfragment

79dc372

Fix test

924ac98

use default moduleFileExtensions and add ee.js and ce.js so that jest…

7dd6c14

… can read those extensions in peregrine

larsroettig requested a review from revanth0212 February 11, 2021 18:38

larsroettig marked this pull request as ready for review February 11, 2021 18:39

Merge branch 'develop' into refactoringPricesummaryfragment

b47f3b3

m2-assistant bot mentioned this pull request Feb 11, 2021

[chore/feature] Move PriceSummaryFragment to peregrine #3010

Closed

sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Feb 11, 2021

revanth0212 requested changes Feb 16, 2021

View reviewed changes

m2-community-project bot assigned revanth0212 Feb 16, 2021

m2-community-project bot added Progress: changes requested and removed Progress: ready for review labels Feb 16, 2021

m2-community-project bot assigned sirugh Feb 16, 2021

m2-community-project bot added Progress: review and removed Progress: changes requested labels Feb 16, 2021

Chnage jest.config

108fdc9

sirugh reviewed Feb 17, 2021

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

sirugh reviewed Feb 17, 2021

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

Lars Roettig and others added 2 commits February 17, 2021 22:12

Update jest.config.js

8fcb1e4

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

Update jest.config.js

2d850b7

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

revanth0212 previously approved these changes Feb 18, 2021

View reviewed changes

m2-community-project bot added Progress: approved and removed Progress: review labels Feb 18, 2021

fix format

ca2bdaf

larsroettig dismissed revanth0212’s stale review via ca2bdaf February 18, 2021 17:54

larsroettig requested review from revanth0212 and sirugh February 19, 2021 10:00

revanth0212 approved these changes Feb 19, 2021

View reviewed changes

Merge branch 'develop' into refactoringPricesummaryfragment

de23661

dpatil-magento merged commit 8697834 into magento:develop Feb 20, 2021

m2-community-project bot added Progress: done and removed Progress: approved labels Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move PriceSummaryFragment to peregrine #3007

Move PriceSummaryFragment to peregrine #3007

larsroettig commented Feb 10, 2021 •

edited by sirugh

Loading

PWAStudioBot commented Feb 10, 2021 •

edited

Loading

larsroettig commented Feb 11, 2021

sirugh commented Feb 11, 2021

revanth0212 left a comment

revanth0212 Feb 16, 2021

sirugh Feb 16, 2021

revanth0212 Feb 17, 2021

sirugh Feb 17, 2021

revanth0212 Feb 16, 2021

sirugh Feb 16, 2021

revanth0212 Feb 17, 2021

sirugh Feb 17, 2021

sirugh Feb 17, 2021

revanth0212 Feb 17, 2021

larsroettig commented Feb 17, 2021

revanth0212 left a comment

larsroettig commented Feb 19, 2021 •

edited

Loading

Move PriceSummaryFragment to peregrine #3007

Move PriceSummaryFragment to peregrine #3007

Conversation

larsroettig commented Feb 10, 2021 • edited by sirugh Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

Resolved issues:

PWAStudioBot commented Feb 10, 2021 • edited Loading

larsroettig commented Feb 11, 2021

sirugh commented Feb 11, 2021

revanth0212 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsroettig commented Feb 17, 2021

revanth0212 left a comment

Choose a reason for hiding this comment

larsroettig commented Feb 19, 2021 • edited Loading

larsroettig commented Feb 10, 2021 •

edited by sirugh

Loading

PWAStudioBot commented Feb 10, 2021 •

edited

Loading

larsroettig commented Feb 19, 2021 •

edited

Loading