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

Tracked Maps and Sets #577

Open
wants to merge 2 commits into
base: master
from
Open

Tracked Maps and Sets #577

wants to merge 2 commits into from

Conversation

@pzuraq
Copy link
Contributor

pzuraq commented Jan 10, 2020

Rendered

pzuraq added 2 commits Jan 3, 2020
@jessica-jordan jessica-jordan mentioned this pull request Jan 10, 2020
10 of 28 tasks complete
@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 11, 2020

I'd prefer this to be in a separate package rather than part of @glimmer/tracking so that we can tree shake it.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

NullVoxPopuli commented Jan 11, 2020

I'd prefer this to be in a separate package rather than part of @glimmer/tracking so that we can tree shake it.

tree shaking can work on multiple imports of the same package

@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 11, 2020

I'd prefer this to be in a separate package rather than part of @glimmer/tracking so that we can tree shake it.

tree shaking can work on multiple imports of the same package

I am unaware of how this works. Right now we can only do "manual tree shaking" in ember-cli; i.e. not load a package. While Embroider may change this someday, we must work with what we have now. Once Ember core is property separated into real separate packages, with today's ember-cli we will be able to only load or not load the entire package.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 11, 2020

This would not be implemented as a separate package either way, so manual treeshaking would not be possible. We should build for automatic treeshaking.

@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 11, 2020

Sorry, why not? It is in a separate addon now.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 11, 2020

@glimmer/tracking is not a separate addon. You can install the addon for types, but it is still provided by ember-source.

@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 11, 2020

Yes. I would like this to be separate from @glimmer/tracking

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

NullVoxPopuli commented Jan 11, 2020

@Gaurav0 I think we have too many imports as is.

@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 11, 2020

@NullVoxPopuli Why? The imports went through the RFC process and were agreed to by the community.

@NullVoxPopuli

This comment has been minimized.

Copy link
Contributor

NullVoxPopuli commented Jan 11, 2020

@Gaurav0 I just mean that I feel like we have too many imports as is -- not that there is anything we can do about it -- all the imports make sense.
Adding more tracked utilities should be on @glimmer/tracking, as the RFC states, because they are very related. Splitting that up doesn't make sense to me.
I understand the desire to have tree shaking, but we need to design for the future, and not hold ourselves back with existing tech... idk if that makes sense. I need to eat. lol

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 11, 2020

@Gaurav0 for pragmatic purposes, this would not be implemented as a separate addon for the time being, which is my overall point.

@Gaurav0

This comment has been minimized.

Copy link
Contributor

Gaurav0 commented Jan 11, 2020

Adding more tracked utilities should be on @glimmer/tracking, as the RFC states, because they are very related. Splitting that up doesn't make sense to me.

IMHO, I disagree. The tracked function and decorator is likely to be much more widely used than any additional utilities. This is similar to how computed is on @ember/object and macros are on @ember/object/computed.

The only additional thing I think should be added to the @glimmer/tracking import may be the @action or @bound decorator as discussed in issue #547

@marcoow marcoow mentioned this pull request Jan 15, 2020
@sandstrom

This comment has been minimized.

Copy link

sandstrom commented Jan 20, 2020

EDIT:

Seems like I have misunderstood the role of proxies here. I still think support for IE 11 should be dropped, but that isn't relevant to this discussion.

I'm very much in favour of this RFC!


PREVIOUS COMMENT:

Good suggestion, I really like this idea!

However, I think designing something proxy-based would be much better and browser support is excellent. Let's come up with something modern, with longer shelf-life.

IE 11 is in rapid decline and only have a few percentage points of usage these days.

Many large software companies has already stopped supporting it, for example:

The days where IE11 was the only browser installed on a computer are long gone.

Ember should drop IE 11 support and focus time on using the modern JS toolset to provide a nice framework. Any Ember user that need to extend support a while longer, e.g. end of 2020, could simply linger on an Ember LTS a while longer. It doesn't need to hold back new framework development.

Regarding the performance drawback with proxies, I don't think it would be a big problem under most real-world scenarios.

@pzuraq

This comment has been minimized.

Copy link
Contributor Author

pzuraq commented Jan 20, 2020

@sandstrom these classes do not require a proxy to wrap transparently, since all access is done through methods, unlike arrays. Ultimately, the exact implementation should likely be whatever is most performant, and whether or not that is a subclass or a proxy should be an implementation detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.