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

Give the initial action a Symbol type #114

Closed
wants to merge 1 commit into from
Closed

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 16, 2015

I intend to use Symbol types for “internal” (or devtools-related) actions that the app should not handle.

As the first change, I propose to give a Symbol type to the initial action used to bootstrap the Store. (Currently it has no type at all.)

I can't imagine a situation in which a Store would want to know whether it's the initial action or not. So I don't think it's needed to expose this Symbol.

@acdlite
Copy link
Collaborator

acdlite commented Jun 16, 2015

Not opposed but why is this better than type being undefined? Just so devtools know it's the first action?

@gaearon
Copy link
Contributor Author

gaearon commented Jun 16, 2015

Yeah it's kind of looking weird there with {} on every hot reload.
Some actual name helps understand what's going on.
Symbol prevents collisions with user constants.

@acdlite
Copy link
Collaborator

acdlite commented Jun 16, 2015

Only "downside" is now Redux depends on a Symbol polyfill. But I guess we're assuming most people are using Babel, anyway.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 16, 2015

Yeah I wonder how bad of an impact that is..
I haven't measured.

@chicoxyzzy
Copy link
Contributor

👎
More then that IMO we shouldn't even use babel-runtime. For example if you use promises through babel-runtime that means people will use non-global promise polyfill no matter does they have native promises or not or does developer who uses Redux had already polyfilled promises. So I believe we can live without Symbols and add caveats for developers that Redux uses promises and they should use polyfill for old browsers (if we need promises in Redux at all)

@chicoxyzzy chicoxyzzy mentioned this pull request Jun 16, 2015
@gaearon
Copy link
Contributor Author

gaearon commented Jun 16, 2015

We don't plan to use Promises in core.

The only runtimey feature I think of using is Symbols.
Is its polyfill large or somehow problematic? Does it drag other polyfills? To my knowledge, it doesn't..

Let's measure the impact on the compiled size, then consider.

@acdlite
Copy link
Collaborator

acdlite commented Jun 16, 2015

👍 for measuring things :D

But let's also weigh the increased size against what it's buying us. If it's just so it looks less "weird" when hot reloading then I'm inclined to agree with @chicoxyzzy.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 16, 2015

It's still part of a bigger picture (#113). It's not the only place where I'd like to use Symbols.
Also, there are talks of React using Symbols for context.

I think they solve the idea of “scoped” state keys or action types in the most natural way possible so it's not just a matter of “looking weird” but also a matter of using the right tool for the job vs making excuses “because polyfill”.

But yeah, need to measure first. :-)

@dariocravero
Copy link
Contributor

Here's redux with core-js/es6/symbol (I imagine that's what we need, right?)

redux [deps*] $ npm run browser

> redux@0.11.1 browser /Users/craverod/opensource/dariocravero/redux
> scripts/browser

Hash: 743b11f65ff43f463865
Version: webpack 1.9.11
Time: 785ms
   Asset     Size  Chunks             Chunk Names
redux.js  72.7 kB       0  [emitted]  main
    + 67 hidden modules
Hash: e478af1a46483b53469e
Version: webpack 1.9.11
Time: 1327ms
       Asset     Size  Chunks             Chunk Names
redux.min.js  19.1 kB       0  [emitted]  main
    + 67 hidden modules

Without it:

redux [deps*] $ npm run browser

> redux@0.11.1 browser /Users/craverod/opensource/dariocravero/redux
> scripts/browser

Hash: f70eae790597111e673b
Version: webpack 1.9.11
Time: 754ms
   Asset     Size  Chunks             Chunk Names
redux.js  57.4 kB       0  [emitted]  main
    + 53 hidden modules
Hash: 01bc6a934e4d0dc8e21f
Version: webpack 1.9.11
Time: 1198ms
       Asset     Size  Chunks             Chunk Names
redux.min.js  12.6 kB       0  [emitted]  main
    + 53 hidden modules

It has an impact of 25% in the regular build and almost 35% in the minified one.

@chicoxyzzy
Copy link
Contributor

@gaearon some people have native symbols. I bet you have them too in your Chrome.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 16, 2015

@chicoxyzzy

I understand that.

@dariocravero

Woah, that's a lot! I'm not super sure how core-js works. Can you try compiling code “as is” vs with changes in this PR?

@acdlite
Copy link
Collaborator

acdlite commented Jun 17, 2015

Just so we're on the same page: If we do decide to depend on Symbols, we shouldn't actually bundle it with Redux; we should list it in the docs as a prerequisite a la React, right?

@chicoxyzzy
Copy link
Contributor

can we just add caveat that Redux uses Symbol? so developers will have a choice if they want to use polyfill or not

@dariocravero
Copy link
Contributor

@acdlite true

@dariocravero
Copy link
Contributor

@gaearon as is:

redux [master*] $ ./scripts/browser
Hash: 9ea61aa7adabe3cb35d3
Version: webpack 1.9.11
Time: 840ms
   Asset     Size  Chunks             Chunk Names
redux.js  76.3 kB       0  [emitted]  main
    + 79 hidden modules
Hash: e6a95b50300e7ee1dce2
Version: webpack 1.9.11
Time: 1385ms
       Asset     Size  Chunks             Chunk Names
redux.min.js  20.2 kB       0  [emitted]  main
    + 79 hidden modules

with this PR:

redux [master*] $ ./scripts/browser
Hash: 8cf52255297e21727897
Version: webpack 1.9.11
Time: 853ms
   Asset     Size  Chunks             Chunk Names
redux.js  84.2 kB       0  [emitted]  main
    + 85 hidden modules
Hash: ea0c217c58871fab8a68
Version: webpack 1.9.11
Time: 1503ms
       Asset     Size  Chunks             Chunk Names
redux.min.js  23.2 kB       0  [emitted]  main
    + 85 hidden modules

This code is just running scripts/browser on top of master (that's why the results differ from my previous reports). Anyway, the difference is almost negligible.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 17, 2015

OK, here's what I suggest: let's disable Babel's Symbol polyfilling and have something custom that turns Symbol('STUFF') into "__STUFF__" on browsers that don't support Symbols natively, but uses Symbols if they are supported. (I also want typeof to just work.. need to look how Babel implements that—probably via a plugin?) nah I don't care about typeof.

Is this too crazy?

@chicoxyzzy
Copy link
Contributor

I believe that it's much better to get rid of any kind of polyfill from such lightweight library. So we can just add a note in docs that Redux uses symbol. But... Are you sure Symbol is so necessary here? We lived without symbols in JS for many years. Yes symbols are cool for large-scale applications as they can help implement some cool patterns. But this is a library not a large-scale application.
As I know @goatslacker was using symbols for Alt but than he removed them.
I just walked the same path recently. At work I wrote some library which we use in different projects now. I realized that there is not so much benefits from symbols in small libraries. I'm still using symbols in some complex projects but I'm trying to keep the library as much simple, lightweight and super-performant as possible. This is critical when library might be used for huge SPA's and tiny embeddable widgets at same time. It's very important to be sure your library just works everywhere where ES5.1 works.

@chicoxyzzy
Copy link
Contributor

Also do you really want to write tests for all cases when you use Symbol or string constant? Because you can't be 100% sure it will work same way everywhere. What about @acdlite's experiments with Flow? Are you sure Flow works well with Symbol type?

@ooflorent
Copy link
Contributor

23kb minified is nothing. Moreover assets are typically served GZIP-ed.

@goatslacker
Copy link

Symbols can't really be polyfilled well and they're iffy on IE8. FWIW I wrote a very minimal spec compliant Symbol polyfill, it's the one I was using in alt until I removed Symbols: https://github.com/goatslacker/es-symbol

My 2 cents is that it's probably not worth it until there's more ES6 adoption, just try and pick a really mangled constant that no one will ever use.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 17, 2015

Uh, nevermind, I'll just prefix internal types with an underscore for now.
Will create a new PR.

@gaearon gaearon closed this Jun 17, 2015
@gaearon gaearon deleted the give-it-a-type branch June 17, 2015 11:29
@ooflorent
Copy link
Contributor

Double @ is also nice.

const somePropSymbol = '@@someProp'
const someProp = this[somePropSymbol]

@gaearon
Copy link
Contributor Author

gaearon commented Jun 17, 2015

Good point!

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

Successfully merging this pull request may close these issues.

6 participants