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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Rename devToolsExtension to something more explicit #99

Closed

Conversation

calebmer
Copy link

The first qualification for #1675, making the name window.devToolsExtension less general and more explicit. I chose __REDUX_DEVTOOLS_EXTENSION__ because that's the name of this package over __REDUX_DEVTOOLS_ENHANCER__ or something else because the object contains a few extra important methods like open(). The name window.devToolsExtension was deprecated instead of removed altogether. If someone tries to use it, they will get a warning but it will work as before.

I'm afraid I don't know how to test if this doesn't break things 馃槓

@calebmer calebmer force-pushed the refactor/explicit-window-name branch from ae6326b to 36f1263 Compare April 24, 2016 14:23
@zalmoxisus
Copy link
Owner

Will come back to this after reduxjs/redux#1702.

Anyway I'd prefer to just add an alias to __REDUX_DEVTOOLS_EXTENSION__, instead of renaming window.devToolsExtension, as we want to extend the extension to be used not only with redux, it's just about sending data to the extension and subscribing to it.

Thanks for contributing, I appreciate it a lot, and sorry for taking it so long!

@lukescott
Copy link

Any reason it couldn't be just window.reduxDevToolsExtension?

__REDUX_DEVTOOLS_EXTENSION__ screams constant to me. And there is nothing wrong with window.devToolsExtension per-se, other than it's not obvious that it's for Redux.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jul 24, 2016

__REDUX_DEVTOOLS_ENHANCER__ could be a shortcut for window.reduxDevToolsExtension() as in most of the cases, the function is used without arguments, though it could be used for advanced usage and API.

So, we could use:

let store = createStore(reducer, window.__REDUX_DEVTOOLS_ENHANCER__);

instead of

let store = createStore(reducer, window.devToolsExtension && window.devToolsExtension());

and

let store = createStore(reducer, initialState, compose(
  applyMiddleware(...middleware),
  window.__REDUX_DEVTOOLS_ENHANCER__ || f => f
));

instead of

let store = createStore(reducer, initialState, compose(
  applyMiddleware(...middleware),
  window.devToolsExtension ? window.devToolsExtension() : f => f
));

On one hand, window.devToolsExtension is confusing as it is not clear what this global variable stands for. On the other hand, the extension is not intended to be used only for redux. See react-counter and react-counter-messaging examples.

Since the purpose of this PR cannot be achieved without zalmoxisus/redux-devtools-instrument#1 and reduxjs/redux#1702, I'd open it for discussions and suggestions.

@zalmoxisus zalmoxisus changed the title Rename devToolsExtension to something more explicit [RFC] Rename devToolsExtension to something more explicit Jul 24, 2016
@lukescott
Copy link

Is Redux still a dependency either way? If so, reduxDevToolsExtension makes the most sense to me. devToolsExtension can mean anything. If you can add to it, I think reduxDevToolsExtension doesn't impede that ability.

__REDUX_DEVTOOLS_ENHANCER__ seems like a constant. Like a primitive value (string, boolean, number). Calling window.devToolsExtension() produces another function right? __REDUX_DEVTOOLS_ENHANCER__ just look like a function to me.

Given that a rename will probably happen, how about:

createStore(reducer, window.reduxDevToolsExtension);

And:

createStore(reducer, window.reduxDevToolsExtension && window.reduxDevToolsExtension.configure(...));

This is what my code currenlty looks like:

export default function configureStore(initialState) {
    if (process.env.NODE_ENV === "production") {
        return createStore(rootReducer, initialState);
    } else {
        const devTools = window.devToolsExtension && window.devToolsExtension();
        const store = createStore(rootReducer, initialState, devTools);
        if (module.hot) {
            module.hot.accept("../reducers", () =>
                store.replaceReducer(require("../reducers").default)
            );
        }
        return store;
    }
}

Which ends up being compiled to this because of webpack for production code:

export default function configureStore(initialState) {
    return createStore(rootReducer, initialState);
}

The && thing is far better than what it used to look like:

const store = (window.devToolsExtension ?
    window.devToolsExtension()(createStore) : createStore
)(rootReducer, initialState);

@sompylasar
Copy link

sompylasar commented Sep 1, 2016

__REDUX_DEVTOOLS_ENHANCER__ seems like a constant. Like a primitive value (string, boolean, number).

It looks like a super-global. And constants may have non-primitive values as well, so this is a constant global enhancer (pre-created for you with the default options).

There might be another constant next to it, __REDUX_DEVTOOLS_ENHANCER_FACTORY__ which you would call to make a local enhancer with custom options:

const devToolsEnhancerFactory = window.__REDUX_DEVTOOLS_ENHANCER_FACTORY__;
const devToolsEnhancer = ( devToolsEnhancerFactory && devToolsEnhancerFactory({ any: "option" }) );
createStore(reducer, devToolsEnhancer);

@sompylasar
Copy link

__REDUX_DEVTOOLS_ENHANCER__ could be a shortcut for window.reduxDevToolsExtension()

Please do not pollute the global namespace with more entities than absolutely required, or name them in a globally unique fashion.

@zalmoxisus
Copy link
Owner

Implemented. Closing in favour of #220.

@zalmoxisus zalmoxisus closed this Oct 4, 2016
@calebmer calebmer deleted the refactor/explicit-window-name branch October 7, 2016 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants