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

Opts are being shared across all tag instances #2613

Closed
fabien opened this issue Jul 21, 2018 · 6 comments

Comments

@fabien
Copy link
Contributor

commented Jul 21, 2018

Help us to manage our issues by answering the following:

  1. Describe your issue:

If you are passing an object to mount('*', { ... }) this object is now shared by reference with all tags.

This is a undesirable side-effect of the small change, introduced by #2581 - and I would say it's a bug, or at least a breaking change. It trades a fix for an 'edge case' situation (why would the object passed be an observable by itself, and not just a member of a plain object that is being passed down?), for a far more far-reaching issue that it now brings up.

  1. Can you reproduce the issue?

https://plnkr.co/edit/ZwmBRx6LtUEBz1OXCu7K?p=preview

  1. On which browser/OS does the issue appear?

N/A

  1. Which version of Riot does it affect?

Starting with 3.10.0 and commit 244cbb5

  1. How would you tag this issue?
  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@Svish

This comment has been minimized.

Copy link

commented Sep 2, 2018

Was going to ask about this... trying to learn how to use Riot, following a YouTube tutorial, and got to this point: https://youtu.be/al87U6NgRTc?t=8m49s

The last tag of his is empty, while mine gets the value of the previous tag.

<!-- tags/hello-world.tag -->
<hello-world>
	<p>Hello { opts.greet }</p>
</hello-world>

<!-- index.html -->
<html>
	<h1>Let's Learn?</h1>

	<hello-world></hello-world>
	<hello-world greet="John"></hello-world>
	<hello-world></hello-world>

	<script src="bower_components/riot/riot.min.js"></script>
	<script src="tags/hello-world.js"></script>
	<script>
		riot.mount('hello-world', {greet: 'Bob'});
	</script>
</html>

Result:
capture

Expected the last one to be bob as well... 🤔


riot-cli: 4.1.0
riot-compiler: 3.5.1

@GianlucaGuarini GianlucaGuarini added bug critical and removed to verify labels Sep 2, 2018

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

I have fixed the problem without introducing breaking changes! You will be able to workaround this issue in the next riot minor version simply passing the options argument as function. In that case the object created will be not shared across different instances.

riot.mount('hello-world', function() {
   return { greet: 'Bob' }
})
@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

riot@3.12.0 is out, thanks for your feedback

@fabien

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018

Hi Gianluca, thanks for looking into this!

However, I feel that a breaking change was introduced unintentionally before, and therefor I would’ve expected a fix to revert to the known/old/expected behavior (as proven by the comment by Svish).

For this reason, I feel it would’ve been more appropriate to introduce a flag/option to enable/disable a certain behavior we’d all agree on as being the expected one. Perhaps further discussion was useful before pushing out a release so quickly.

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

@fabien unfortunately there is no way to clone the opts object internally. Some users will use as options riot.observable some others classes... so that's the safest and cleanest way to handle them.
The 244cbb5 was a bugfix for an issue that has always been in riot since its v2. I have corrected the wrong behavior, I have provided an elegant solution to handle all the possible use cases and I have boosted the riot performances. I am sorry but I don't see any margin for discussion here. ✌️

@JuanLuisClaure

This comment has been minimized.

Copy link

commented Sep 13, 2018

Just in time Sir @GianlucaGuarini , because that was an awful bug,it keeps me wake at night. In this issue you named riot.tag() as solution, is good as far with string. but when use array doesnt work. ¿so this last implementation is a better solution for hydratation-html-thing from server? it works for me. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.