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

Remove some js/src/external-components #643

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented May 20, 2021

Changes proposed in this Pull Request:

This is a cleanup task, as the fixes for forked components were released.

Screenshots:

Detailed test instructions:

According to the woocommerce/components/CHANGELOG.md#620 the majority of changes were around Search, SummaryNumber and <Filter> components. So report pages are at the biggest risk, to be broken.

  1. Go to report pages, or any other you may suspect :|
  2. Check that it works as before.

Changelog Note:

Removed forked @woocommerce/components.

Additional notes:

  • Removing external-components makes it easier for us to catch up with other dependencies. For example, the latest @woocommerce/eslint-plugin complains a lot about external dependencies and experimental components being used. We have those problems also in our components, but this PR reduces a few.

@tomalec tomalec added the type: technical debt This issue/PR represents/solves the technical debt of the project. label May 20, 2021
@tomalec tomalec self-assigned this May 20, 2021
@jconroy
Copy link
Member

jconroy commented May 21, 2021

Does this have any implications for our support of older versions of wc/wc-admin/wp?

@tomalec
Copy link
Member Author

tomalec commented May 21, 2021

Does this have any implications for our support of older versions of wc/wc-admin/wp?

Ha! Good question, I haven't thought about it :| Is there some kind of compatibility table between @woocommerce-admin/{package} and Woo?

The README of the entire wc-admin, states

WordPress 5.4 or greater and WooCommerce 4.8.0 or greater

(for us it's "- WordPress 5.3+ - WooCommerce 4.5+" )
So it may be affected. But what is the relation between wc-admin as a whole, and just /components?


I did some digging and it seems that they bumped WP to 5.4 at the time of /components@5.1.2 (our current) being used. So even 5.1.3 may be problematic :/

@eason9487
Copy link
Member

eason9487 commented May 21, 2021

Does this have any implications for our support of older versions of wc/wc-admin/wp?

I have WooCommerce 4.9.2 and WooCommerce Admin 2.12. in my local site. If the status of the wc-admin plugin is

  • activated, then it would load /wp-content/plugins/woocommerce-admin/dist/components/index.min.js?ver=2.1.2
  • deactivated, then it would load /wp-content/plugins/woocommerce-admin/dist/components/index.min.js?ver=1.8.3

and both of them have the components@5.1.2 inside. Unfortunately, seems to be incompatible:

image

@jconroy
Copy link
Member

jconroy commented May 24, 2021

WordPress 5.4 or greater and WooCommerce 4.8.0 or greater

Yep as you say, that version bump happened in Feb after we'd kicked things off.

I don't think we can rush this one right now. Let's knock over the reporting issues and then circle back

@tomalec tomalec added priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. status: on hold The issue is currently not prioritized. labels May 24, 2021
@tomalec
Copy link
Member Author

tomalec commented Jun 22, 2021

I guess, it got unlocked after we bumped required versions to

WP 5.5
WC 5.2

in #803

@tomalec tomalec removed the status: on hold The issue is currently not prioritized. label Jun 22, 2021
@tomalec
Copy link
Member Author

tomalec commented Jun 23, 2021

@tomalec tomalec added the status: on hold The issue is currently not prioritized. label Jun 23, 2021
@tomalec
Copy link
Member Author

tomalec commented Jun 23, 2021

@jconroy @eason9487 Do you know why we can't use our own version of /components and we have to share it with wc-admin?

I thought that was the main advantage of React over Web Components, that using different versions of the same component is easy and seamless. Is this a technical or policy limitation?

@eason9487
Copy link
Member

eason9487 commented Jun 24, 2021

Do you know why we can't use our own version of /components and we have to share it with wc-admin?

I guess it's a consideration of the overall bundle size of js files. But actually, we're still using our own specific version of @wordpress/components, maybe it's not a mandatory restriction?

Also, if we can use our own specific dependency versions, there should be fewer compatibility issues with different versions of WordPress and WooCommerce.

@eason9487 eason9487 changed the base branch from trunk to develop August 25, 2021 03:26
@tomalec tomalec added type: technical debt - dependency extraction This issue/PR suffers from the dependency extraction and management type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies labels Sep 16, 2021
@tomalec
Copy link
Member Author

tomalec commented Nov 25, 2021

I'm rebasing this PR on top of #1110 as there we move to WC5.6+ => @woocommerce/components 8.0.0 7.1.0+ which has the changes we need https://github.com/woocommerce/woocommerce-admin/blob/v2.6.5/packages/components/CHANGELOG.md

as those fixes were released in `@woocommerce/components`:
- woocommerce/woocommerce-admin#6890,
- woocommerce/woocommerce-admin#6062
Remove no longer needed dependencies.
@tomalec tomalec changed the base branch from develop to update/min-wc-5.7 November 25, 2021 12:03
@tomalec tomalec removed the status: on hold The issue is currently not prioritized. label Dec 1, 2021
@tomalec tomalec requested a review from a team December 1, 2021 14:51
@tomalec tomalec changed the title Remove js/src/external-components Remove some js/src/external-components Dec 1, 2021
Base automatically changed from update/min-wc-5.7 to develop December 2, 2021 18:04
Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

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

Tested with WC 5.7 and 5.9, and there are no errors in either.

🚧
However, an inappropriate falsy check was later added to the <FilterPicker> component, resulting in the free listings program ID 0 being cleared from the URL query programs when filtering single program by free listings program.

Kapture.2021-12-03.at.13.28.38.mp4
Before: page=wc-admin&reportKey=programs&path=%2Fgoogle%2Freports&filter=single_program&programs=0
After:  page=wc-admin&reportKey=programs&path=%2Fgoogle%2Freports&filter=single_program

One relevant hidden influence is that the isSubset check here will not be true, but in terms of results, it will still fall into the else logic, making the correct rendering result of single program filter. It can be reproduced by refresh the page with path /wp-admin/admin.php?page=wc-admin&reportKey=programs&path=%2Fgoogle%2Freports&filter=single_program.

// If it's a subset of static values, resolve it immediately.
if ( isSubset( ids, freeListingsProgramsIds ) ) {
programs = freeListingsPrograms;
} else {
// If there are any paid programs, resolve it once we get it.
programs = ( await promiseProgramsList ).filter( ( campaign ) =>
ids.has( campaign.id )
);
}

Another influence is that it still queries the APIs for paid programs and shows incorrect reporting results.

2021-12-03 13 49 33


📜 Probably solution

Fix by WC-admin will need to continue waiting for the next L-2 to include the patch. To move forward with this PR, I tried changing FREE_LISTINGS_PROGRAM_ID constant to -1 and it seems to work correctly, but I didn't check if there are new side effects afterward.

@tomalec
Copy link
Member Author

tomalec commented Dec 3, 2021

Thanks for finding that!

This looks almost like a regression for woocommerce/woocommerce-admin#7028

I think it really unfortunate, we cannot use either the old or new version of @woocommerce/components.FilterPicker.
Maybe we need to PR the full tests coverage for the component to wc-admin repo, before we start using it here :|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. type: technical debt - dependency extraction This issue/PR suffers from the dependency extraction and management type: technical debt - external devX This issue/PR suffers from the ergonomics of external tools/dependencies type: technical debt This issue/PR represents/solves the technical debt of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search component does not accept static options Array in autocomplete
3 participants