Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

regression: observable.map no longer updates observers in 1.3.0 #143

Closed
pdeva opened this issue May 1, 2019 · 13 comments
Closed

regression: observable.map no longer updates observers in 1.3.0 #143

pdeva opened this issue May 1, 2019 · 13 comments
Labels
bug Something isn't working

Comments

@pdeva
Copy link

pdeva commented May 1, 2019

sample code

       @observable
       filterValues = observable.map({}, {deep: false, name: "filterValues"});

        @action
	setFilterValue(filterName: string, values: string[])
	{
		const strings = values.filter(v => v != null);
		this.filterValues.set(filterName, strings);
	}

this works properly and updates listening components in 1.2.0 but does not update them in 1.3.0

@pdeva pdeva changed the title regression: observable.map no longer updates in 1.3.0 regression: observable.map no longer updates observers in 1.3.0 May 1, 2019
@danielkcz
Copy link
Collaborator

danielkcz commented May 1, 2019

Um, I don't see any mobx-react-lite related code in here. Please provide codesandbox with the actual issue. Btw, I don't think you should be using decorator and function call (observable.map) together. It's either this or that.

@danielkcz danielkcz added the question Further information is requested label May 1, 2019
@pdeva
Copy link
Author

pdeva commented May 1, 2019

@FredyC the reason its mobx-react-lite related is because just changing mobx-react-lite from 1.2.0 to 1.3.0 causes the issue to surface.

i removed the decorator from the observable.map field, and yes it does work normally in 1.2.0 but the issue remains in 1.3.0.

not sure how to create a sandbox for this, since our codebase is so huge and complicated. is there any info, piece of code i can provide that can help get to the bottom of this?

@danielkcz
Copy link
Collaborator

danielkcz commented May 1, 2019

Sorry, cannot help it until you show the code that's related to the package or at least some minimal repro of the problem.

@danielkcz danielkcz added the wontfix This will not be worked on label May 1, 2019
@pdeva
Copy link
Author

pdeva commented May 1, 2019

here is some code that uses the mobx-react-lite observer to access the filterValues field shown in the OP:

const EntityFilter = observer(({column}: IFilterValueFetcherProps) => {

        //Another observable accessed here : 'filters'
	const defs = clusterFilterStore.filters.filter(f => f.column == column);

	let initialChecked = true, initialSelected: string[] = [];
	if (defs.length > 0)
	{
		initialChecked = defs[0].include;
		initialSelected = defs[0].items.slice(); //slice cause its a proxy
	}

	const [isIncludedChecked, setIsIncludedChecked] = useState(initialChecked);
	const [selectedItems, setSelectedItems] = useState<string[]>(initialSelected)

        //USES filterValues here
	const list = cubeStateStore.filterValues.get(column);
	const vals: string[] = list ? list : [];
	const filteredItems = vals.filter(it => !selectedItems.includes(it));

	return (
		<>
			{
				(cubeStateStore.cluster && cubeStateStore.entity) &&
				<FilterValueFetcher key={column} column={column}/>
			}
			<Radio.Group value={isIncludedChecked} onChange={onRadioGroupChange}>
				<Radio value={true}>Include</Radio>
				<Radio value={false}>Exclude</Radio>
			</Radio.Group>
			<Select mode="multiple" onChange={onChange} value={selectedItems}>
				{filteredItems.map(v => <Select.Option key={v}>{v}</Select.Option>)}
			</Select>
		</>
	)
});

@mweststrate
Copy link
Member

mweststrate commented May 1, 2019 via email

@meyer
Copy link

meyer commented May 1, 2019

I’m experiencing a similar regression. Will try to get a repro up shortly.

@danielkcz
Copy link
Collaborator

I think I do smell a culprit. At first, I thought that only PR #130 has landed in 1.3.0, but there was also #119 which I completely forgot about and was mostly experimental.

I've published mobx-react-lite@test which is the same as 1.3.0, but that change is removed. Please do test out your issues with this version and let me know if it helps anything. And of course, if you would be willing to provide reproduction, it would be useful to improve our test cases and prevent such issue in the future.

@danielkcz danielkcz reopened this May 1, 2019
@danielkcz danielkcz added bug Something isn't working and removed question Further information is requested wontfix This will not be worked on labels May 1, 2019
@meyer
Copy link

meyer commented May 1, 2019

@FredyC can do, trying now…

@meyer
Copy link

meyer commented May 1, 2019

@FredyC that seems to fix the issue!

@danielkcz
Copy link
Collaborator

@meyer Glad to hear that. As I said, it would be great to see some example of what was actually broken so it won't happen again the future.

@meyer
Copy link

meyer commented May 1, 2019

@FredyC i’m going to try extract a repro out tonight. Do you have a recommended way for me to debug? I’ve been using mobx dev tools on the working version and the non-working version to see if there are any differences, but there’s probably a better method of debugging.

@danielkcz
Copy link
Collaborator

I don't really need you to debug anything. Just try to show some minimal example of the issue with version 1.3.0. Of course, you can also submit PR with an added test case if you like :)

@danielkcz
Copy link
Collaborator

I've published official 1.3.1 with the fix. I will close this now and further discussion can continue in #144 which has at least some viable example. This probably wasn't ever related to observable.map alone.

@mobxjs mobxjs locked as resolved and limited conversation to collaborators May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants