LPS-139246 - Create custom autocomplete for object-dynamic-data-mapping #1471
Conversation
|
Please only forward necessary changes to Brian Chan during stabilization. Nonurgent changes should wait until the ongoing DXP 7.4 GA1 and Portal 7.4 GA4 release has been completed. For more details on the release timeline and status, see product-delivery. |
|
CI is automatically triggering the following test suites:
|
❌ ci:test:sf - 0 out of 1 jobs passed in 2 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-139246 1 Failed Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#246 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#1471 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - bryceosterhaus > liferay-frontend - PR#1471 - 2021-09-15[15:31:12] Testray Importer:publish-testray-report#732 |
| <li> | ||
| <div ref={ordersListRef} /> | ||
| </li> |
There was a problem hiding this comment.
This was a very strange way to do this. I need to properly test this in the UI but I really don't like this pattern being used. Effectively this was an anti-pattern here of bi-directional data flow. If the Autocomplete component was passed a customView prop, instead of rendering inside of the Autocomplete component it was handling all the business logic of requesting items and then rendering them via createPortal to this node. The reason this pattern is bad is because Autocomplete is now rendering to an unrelated area. If we want to properly do this, we must pull the state(items) up the react tree and outside of Autocomplete. That way we either pass the items to Autocomplete or we render them here.
There was a problem hiding this comment.
cc @FabioDiegoMastrorilli
I noticed you added this API in liferay@4aa3853#diff-fbc58da25a23f789ae15c90776dd7b5ce184557ea3d17369a143dfb8bb7554bcR270
Can you verify my changes in this PR and see how we can get around this pattern of using the portal to render the content on an outside node.
There was a problem hiding this comment.
Hi @bryceosterhaus it makes totally sense :)
Passing a ref there would help in case we would like to use all the autocomplete functionalities (search, infinite scrolling, custom views) to render a list of results even out of a react context. In that case there won't be any state tree so it wont't look like an unsafe operation.
That being said... please have a look at the AccountSelector component. I've use the Autocomplete there to avoid code duplication and, if I could still use it there to get the same result, it would be great.
| const node = useRef(); | ||
| const dropdownNode = useRef(); | ||
| const inputNode = useRef(); | ||
| const FetchedCustomView = useLiferayModule(props.customViewModuleUrl); |
There was a problem hiding this comment.
I removed this part and the customViewModuleUrl prop because I didn't see this being used anywhere
There was a problem hiding this comment.
That was a requirement from @marco-leo to let external developer using their own views by passing their module url.
We do something similar here - I created the hook useLiferayModule during the migration so it's not used there, I think it can be moved out of commerce modules for better reusability.
We don't use it internally but somebody theoretically could.
NB:
I just realised we have the same useLiferayModule function declared twice 🤦
|
cc @marco-leo, I think you originally authored this Autocomplete component and want to make sure you see this. Brian had requested that we move this component to frontend-js since it is being referenced and used in the objects module, here. If you think we need to change anything here, let me know |
❌ ci:test:stable - 10 out of 11 jobs passed❌ ci:test:relevant - 21 out of 25 jobs passed in 2 hoursClick here for more details.This pull is eligible for reevaluation. When this upstream build has completed, using the following CI command will compare this pull request result against a more recent upstream result: ci:reevaluate:1297401_283 Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 601787d738f00717211a50876d0873ebabcd8035 ci:test:stable - 10 out of 11 jobs PASSED10 Successful Jobs:
ci:test:relevant - 21 out of 25 jobs PASSED4 Failed Jobs:21 Successful Jobs:
For more details click here.Failures unique to this pull:
Failures in common with acceptance upstream results at 601787d:
Test bundle downloads:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#283 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#1471 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - bryceosterhaus > liferay-frontend - PR#1471 - 2021-09-15[15:31:10] Testray Importer:publish-testray-report#846 |
| url.searchParams.append('pageSize', pageSize); | ||
| } | ||
|
|
||
| return Liferay.Util.fetch(url, { |
There was a problem hiding this comment.
You can import it from frontend-js-web together with {debounce}. It would be great if, together with [no-global-fetch](https://github.com/liferay/liferay-frontend-projects/blob/master/projects/eslint-config/plugins/portal/lib/rules/no-global-fetch.js) we could make sure that we use the imported one when in a module context
|
|
||
| return Liferay.Util.fetch(url, { | ||
| headers: new Headers({ | ||
| Accept: 'application/json', |
There was a problem hiding this comment.
I've seen this a couple of times around already. Wondering if we'd want to:
- Create some type of
fetchJSONutility on top of ourfetch(probably not, but it's an option) - Provide something like
FETCH_HEADERS_JSON
|
|
||
| getData(apiURL, query, 1, 10) | ||
| .then((jsonResponse) => { | ||
| updateItems(jsonResponse.items); |
There was a problem hiding this comment.
Won't this be unsafe from an isMounted point of view? By the time this runs, something else could've happened. I think we have useStateSafe for this, which will be rendered useless by facebook/react#22114 anyways
There was a problem hiding this comment.
Yeah we probably should use useStateSafe for this and I will update it.
From a memory perspective, this isn't a terrible issue because it likely would only happen once and would not be a re-occuring memory leak like an event listener would be. Its sort of "memory splash"™️ 😂
There was a problem hiding this comment.
Actually the more I think about it, and going off what Dan says
This will trigger the setState warning. However, there is no actual problem here. There's no "memory leak" here: the POST comes back, we try to set state, the component is unmounted (so nothing happens), and then we're done. Nothing keeps holding onto the component indefinitely.
I think we are fine to ignore this case for now and not go overboard with the useSafeState for cases that don't "hold onto the component indefinitely"
| const [query, setQuery] = useState(initialLabel || ''); | ||
|
|
||
| const [selectedItem, updateSelectedItem] = useState(initialValue || value); | ||
| const [items, updateItems] = useState(null); |
There was a problem hiding this comment.
Is this where Require that React.useState returned arguments has consistent naming pattern comes from? 😁
Are you planning on fixing this later in the process of the PR, or wait for the rule to be enforced?
There was a problem hiding this comment.
@jbalsas I was waiting to enforce is until the rule is in, and thats sort of the pattern I am following for things right now. If I can create an eslint rule for it, I don't bother leaving that in my review yet because I that way we can just make that change all at once.
For this code specifically, I just copied it from the old implementation and didn't get the standards too much thought at the time, but as I go through it now I am updating it and creating any issues for rules we need for eslint.
| Bundle-Version: 1.0.0 | ||
| Web-ContextPath: /object-dynamic-data-mapping |
There was a problem hiding this comment.
Non-web modules don't usually have a Web-ContextPath (with some exceptions like -taglib). Apparently -form-field-type are also allowed. Not sure if Brian will go for this, but I think it might be safer to follow one of these and split the package:
| @@ -47,7 +47,7 @@ | |||
|
|
|||
| @Override | |||
| public String getModuleName() { | |||
| return "commerce-frontend-js/components/autocomplete/Autocomplete"; | |||
| return "object-dynamic-data-mapping/Autocomplete"; | |||
There was a problem hiding this comment.
I don't see how this could really work. If I'm not mistaken, this relies on object-dynamic-data-mapping/Autocomplete to be aliased and exposed as a global module. This needs to happen via .npmbridgerc, which commerce-frontend-js does which is not really recommended.
As long as you're using a module from the same package, you should be able to use npmResolver to get the right path. See:
There was a problem hiding this comment.
@jbalsas is this even the right way to use a js module at all? Brian wanted this autocomplete component moved out of commerce since it was now referenced in objects, but I wasn't even sure this was the right way for objects to consume a js component.
There was a problem hiding this comment.
@jbalsas is this even the right way to use a js module at all?
Yeah, I think this is legit.
This eventually gets through some layers in ddm until it gets to a Liferay.Loader.require call at data-engine's Field component
In our revisited case, the module is accessing an internal component, which doesn't break our principle of just accessing other modules by their main exports.
In the previous code, objects was accessing commerce's module through a path which should be internal to them. This only worked because of the bridge mechanism that exposes the whole commerce-frontend-js tree. So yeah, accessing external modules by path (digging in their internals) should be persecuted 👊
I'm not sure if I made a good job at answering, so please, let me know if this is still unclear 😂
| labelKey, | ||
| onItemClick, | ||
| valueKey, | ||
| }) => { |
There was a problem hiding this comment.
Should we provide some defaults here, at least for maybe labelKey and valueKey?
I see some disconnect between this and how it is eventually used in this case
<BaseAutocomplete
labelKey={itemsLabel}
valueKey={itemsKey}
/>Which in turn goes up the chain. I think it might be too easy to forget to pass things that are required for the default behaviour to work properly, making the default a bit less useful.
A slight improvement would be to have this APIs typed. Then, at the very least, one could know what's needed ahead of time. #justsayin ;)
There was a problem hiding this comment.
I was trying to change as little as possible on the commerce side which is why there is a disconnect. But now I am just thinking I should re-write the commerce component to be a bit more in-line with our existing patterns.
| required, | ||
| }) { | ||
| const [query, setQuery] = useState(initialLabel || ''); | ||
| const [initialised, setInitialised] = useState(Boolean(CustomView)); |
There was a problem hiding this comment.
For some reason this caught my eye... I didn't know we were switching to british english 😂
There was a problem hiding this comment.
Sometimes its important to pay respects
| import React, {useState} from 'react'; | ||
|
|
||
| function getData(apiUrl, query, page, pageSize) { | ||
| const url = new URL(apiUrl, Liferay.ThemeDisplay.getPortalURL()); |
There was a problem hiding this comment.
There's also just themeDisplay. I don't know which is both, but maybe we should settle for one type of access to the object or another and enforce it as much as we can.
I took a quick look some time ago to see if we could place this elsewhere other than a big global but couldn't come up with anything of substance.
|
|
||
| if (pageSize) { | ||
| url.searchParams.append('pageSize', pageSize); | ||
| } |
There was a problem hiding this comment.
Just a comment here about the use of append vs set. Doing a quick grep in our codebase I see:
searchParams.set: 40 results in 28 filessearchParams.append: 62 results in 24 files
I bring this up because append will always append a new key-value pair regardless of it existing it previously in the URL.
So, if the url happened to be: foo.com?page=1, then append('page', 2) would generate foo.com?page=1&page=2. It would be up to the server then to handle page as a list param, but more often than not, first one will win and the effect of setting the param page to 2 would be negated.
I think in this case it's fairly safe because we are more or less certain themeDisplay.getPortalURL doesn't have search params and we're not passing this url around where others could manipulate it.
I'm not sure if we'd want to do something about this or even how we'd do it, but I get the feeling we're not always being 100% intentional when using these APIs, what could result in hard-to-diagnose issues in the future.
| setPage={setPage} | ||
| setPageSize={setInternalPageSize} | ||
| setSelectedItem={setSelectedItem} |
There was a problem hiding this comment.
I know you're just moving this component to another module/folder but it think it can be changed to a callback or a reducer(depending the depth)
We can see many traverse of state setters everywhere in this code :/
liferay-continuous-integration
added
ci:test:sf - pending
and removed
ci:test:sf - failure
labels
liferay-continuous-integration
added
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-139246 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#78 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#1471 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - bryceosterhaus > liferay-frontend - PR#1471 - 2021-09-20[15:46:55] Testray Importer:publish-testray-report#278 |
| useEffect(() => { | ||
| function handleClick(event) { | ||
| if ( | ||
| autocompleteRef.current.contains(event.target) || | ||
| (dropdownRef.current && | ||
| dropdownRef.current.contains(event.target)) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| setActive(false); | ||
| } | ||
| if (active) { | ||
| document.addEventListener('mousedown', handleClick); | ||
| } | ||
|
|
||
| return () => { | ||
| document.removeEventListener('mousedown', handleClick); | ||
| }; | ||
| }, [active]); |
There was a problem hiding this comment.
This can hopefully be removed later when we expose onClickOutside from clay
There was a problem hiding this comment.
Let's support this, I'm going to create an issue.
| headers: new Headers({ | ||
| Accept: 'application/json', | ||
| 'Accept-Language': themeDisplay.getBCP47LanguageId(), | ||
| 'Content-Type': 'application/json', | ||
| }), |
There was a problem hiding this comment.
@jbalsas I am going to leave this as is for now. I looked into exposing it as a constant with fetch but there are a handful of use cases that need to be thought through. I created a task for this. https://issues.liferay.com/browse/LPS-139556
There was a problem hiding this comment.
Accept and Content-Type are not needed AFAIK if we want to rely on defaults.
| const CustomView = props.customView || FetchedCustomView; | ||
|
|
||
| useEffect(() => { | ||
| if (items && items.length === 1 && props.autofill) { |
There was a problem hiding this comment.
autofill was never actually used so I removed the prop and this effect
| const value = | ||
| selectedItem && getValueFromItem(selectedItem, props.itemsKey); | ||
|
|
||
| if (props.id) { |
There was a problem hiding this comment.
id is also never used so I was able to remove this entire effect
| if (onValueUpdated) { | ||
| onValueUpdated(value, selectedItem); | ||
| } | ||
|
|
||
| if (onChange) { | ||
| onChange({target: {value}}); | ||
| } |
There was a problem hiding this comment.
These props are never used. Meaning <Autocomplete is never used and provided these props.
I am starting to think this component may have just been originally copied from some other place and a bunch of this logic got included. Or perhaps it used to be necessary and was never removed.
There was a problem hiding this comment.
@bryceosterhaus onChange({target: {value}}); is needed to work with DDMForm was just added last week, it required in all DDM forms Fields
There was a problem hiding this comment.
Also onValueUpdated, was created for a purpose (https://issues.liferay.com/browse/COMMERCE-4265) and it's being used here:
6 results - 5 files
modules/apps/commerce/commerce-catalog-web/src/main/resources/META-INF/resources/commerce_catalog/details.jsp
modules/apps/commerce/commerce-channel-web/src/main/resources/META-INF/resources/commerce_channel/general.jsp
modules/apps/commerce/commerce-pricing-web/src/main/resources/META-INF/resources/commerce_price_lists/commerce_price_list/details.jsp
modules/apps/commerce/commerce-product-definitions-web/src/main/resources/META-INF/resources/definition/add_cp_definition.jsp
modules/apps/commerce/commerce-product-definitions-web/src/main/resources/META-INF/resources/definition/duplicate_cp_definition.jsp
There was a problem hiding this comment.
@FabioDiegoMastrorilli thanks for pointing those instances out, I hadn't seen them before. I will check it out.
| updatePage(1); | ||
| updateTotalCount(null); | ||
| updateLastPage(null); | ||
| }, [props.infiniteScrollMode, query]); |
There was a problem hiding this comment.
infiniteScrollMode is never toggled and I moved some of this logic to where query is changed. It is better to update state in a central location rather than updating it once and then updating it again on state change.
1 render is better than 2 renders
| setQuery(val); | ||
| setSelectedItem(null); | ||
| fetchData(); |
There was a problem hiding this comment.
This is pretty weird, I think fetchData is working exactly because of debounce, because we set the query here and it's asynchronous, so we call fetchData right away.
There was a problem hiding this comment.
Oh good point, I thought something seemed off when I was writing this but I just ignored the feeling 😂
| const fetchData = debounce(() => { | ||
| if (isMounted()) { | ||
| setLoading(true); | ||
|
|
||
| getData(apiURL, query, 1, 10) | ||
| .then((jsonResponse) => { | ||
| setItems(jsonResponse.items); | ||
|
|
||
| setLoading(false); | ||
|
|
||
| if (!query) { | ||
| return; | ||
| } | ||
|
|
||
| const found = jsonResponse.items.find( | ||
| (item) => item[labelKey] === query | ||
| ); | ||
|
|
||
| if (found) { | ||
| setSelectedItem(found); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| setLoading(false); | ||
| }); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
I think this could be replaced with useResource.
import {fetch} from 'frontend-js-web';
const [networkStatus, setNetworkStatus] = useState(4);
const {resource: items} = useResource({
fetch,
fetchPolicy: 'cache-first',
link: new URL(apiUrl, themeDisplay.getPortalURL()),
onNetworkStatusChange: setNetworkStatus,
variables: {
search: query,
page: 1,
pageSize: 10
}
});
const initialLoading = networkStatus === 1;
const loading = networkStatus < 4;
const error = networkStatus === 5;There was a problem hiding this comment.
I'll try this out!
There was a problem hiding this comment.
I ended up using this in one place but not the other. I didn't use it for the infinite scroll because I thought it would be odd to refetch and update the list in a useEffect.
There was a problem hiding this comment.
Hmm I think I saw this case in the ListView, maybe we can use, as I understand the rule, it calls fetchData if the value of query changes, in this case with useResource and the query remains available in variables the useResource will automatically request again and thus no longer need useEffect for this. In the onBottomTouched call it's also no longer necessary because you're going to be using page as part of variables too:
const {resource: items} = useResource({
...
variables: {
search: query,
page,
pageSize,
}
});Does it make sense? I can be missing something.
There was a problem hiding this comment.
My concern is that we have to still maintain a list of all items requested since the list grows on scroll. So if we use useResource, we have to use an effect for updating the list. Unless there is another way to join the new requested items with the old requested items because we want to render page 1 + page 2 and not just render one or the other.
const [listItems, setListItems] = useState([]);
const {resource: items} = useResource({
...
variables: {
search: query,
page,
pageSize,
}
});
useEffect(() => {
setListItems([...listItems, ...items]);
}, [items])If we rely on the useEffect, I think that would cause multiple re-renders, once when items is updated with page 2 and then again when we add that with setListItems
There was a problem hiding this comment.
Oh I see, now this makes more sense to me, in fact using useResource wouldn't help in this case, I'm going through the same situation in implementing Tree View for loading more items and maybe useResource will support this via OOTB, still "drawing" as the API would be...
But I agree. Maybe it could just remove the fetchData implementation and shorten the code but still having to use useEffect or useMemo can help too.
| useEffect(() => { | ||
| function handleClick(event) { | ||
| if ( | ||
| autocompleteRef.current.contains(event.target) || | ||
| (dropdownRef.current && | ||
| dropdownRef.current.contains(event.target)) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| setActive(false); | ||
| } | ||
| if (active) { | ||
| document.addEventListener('mousedown', handleClick); | ||
| } | ||
|
|
||
| return () => { | ||
| document.removeEventListener('mousedown', handleClick); | ||
| }; | ||
| }, [active]); |
There was a problem hiding this comment.
Let's support this, I'm going to create an issue.
|
|
||
| return ( | ||
| <> | ||
| <FocusScope> |
There was a problem hiding this comment.
Not totally sure if this is even necessary, it was copied from the original commerce code
| ); | ||
| }; | ||
|
|
||
| function Autocomplete({ |
There was a problem hiding this comment.
Hey @bryceosterhaus looking at this well maybe we can support this component in Clay as a high-level component, it looks like you've eliminated a lot of commerce dependencies here.
There was a problem hiding this comment.
Yeah I was thinking we could investigate this as a next step. My first step is just getting the commerce dependency out of DDM. Once this is merged, I can open a separate issue to investigate either moving this to Clay or taking advantage of our existing autocomplete component
There was a problem hiding this comment.
Yes I agree, it doesn't need to be done now, we can check it out later and we can take care of that too.
…ntend-js-components-web
bryceosterhaus
force-pushed
the
LPS-139246
branch
from
6644513
to
d211506
Compare
|
Just updated with corrections for how the UI behaves in commerce. We ended up not needing the autocomplete component at all. Commerce @marco-leo @FabioDiegoMastrorilli can your team test this out to make sure I didn't miss anything? From my tests I think this should be working as it did before this refactor |
|
Closing in favor of #1489 @FabioDiegoMastrorilli, no rush right now, but I would encourage your team to look into getting rid of those createPortals(ReactPortal) you are using to render the content elsewhere. That sort of pattern isn't super great and will become difficult to maintain over time. I would always recommend opting for a uni-directional data flow. You can check out my work in this commit to see how I got rid of that sort of pattern or feel free to bother me if you need any help in the future. |
https://issues.liferay.com/browse/LPS-139246
This refactors the autocomplete component to be used outside of commerce. I wasn't able to test this in the DXP UI due to backend errors with commerce, but thought I would get this up for review and then test.
I'll leave some comments about parts I moved around