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

FIX: After 'dispose' call in rewind(), handleChange re-fires #2

Closed
wants to merge 1 commit into from

Conversation

snow01
Copy link

@snow01 snow01 commented Jun 19, 2015

After 'dispose' call in rewind(), handleChange re-fires, but this time without any changes. This behaviour resets the value. FIX is to preserve the variables in rewind() method, just before calling the dispose() and using those values to return new head element.

…at has no change, so all set values gets nullified
@KATT
Copy link
Contributor

KATT commented Jun 19, 2015

I'm using this branch, but I couldn't just add the git repo as a dependency in my package.json (because of the build step), so I did the following in my Makefile:

setup: cleanup
    npm install
    # FIXME swap to package.json after https://github.com/nfl/react-helmet/pull/2 is closed
    rm -rf node_modules/react-helmet; cd node_modules && git clone git@github.com:snow01/react-helmet.git && cd react-helmet && npm install

@snow01
Copy link
Author

snow01 commented Jun 19, 2015

Thanks for letting me know that step.

@cwelch5 cwelch5 closed this in fd76e77 Jun 19, 2015
@cwelch5 cwelch5 reopened this Jun 19, 2015
@potench
Copy link
Contributor

potench commented Jun 19, 2015

@snow01 Thanks for submitting this PR. We don't have an Individual CLA form setup yet. Can you email me at engineers@nfl.com, and I'll send you the form to sign manually (you will just reply with an e-signature)? Also, left a comment on the PR, can you take a look at making this adjustment?

@@ -139,12 +139,16 @@ class Helmet extends React.Component {
}

static rewind() {
// after side-effect dispose, handle change fires again - we need to preserve variables before calling dispose.
const _serverTitle = serverTitle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewrote to use ES6 destructuring

        const title = serverTitle;
        const meta = generateTagsAsString(TAG_NAMES.META, serverMetaTags);
        const link = generateTagsAsString(TAG_NAMES.LINK, serverLinkTags);

        this.dispose();

        return {
            title,
            meta,
            link
        };

@cwelch5
Copy link
Contributor

cwelch5 commented Jun 20, 2015

Thanks @snow01 for the PR!

@cwelch5 cwelch5 closed this Jun 20, 2015
@snow01
Copy link
Author

snow01 commented Jun 20, 2015

I see you copied the changes manually rather than merging commits and I do not see my name in contributor to Helmet.jsx.

@snow01
Copy link
Author

snow01 commented Jun 20, 2015

@potench - This CLA is for ? I can make changes, but I believe @cwelch5 has already taken changes manually rather than merging the pull request.

@potench
Copy link
Contributor

potench commented Jun 20, 2015

Normally we wouldn't do that but the helmet code wasn't stable and this fix is a port of same react-document-title fix https://github.com/gaearon/react-document-title/blob/master/index.js
CLA was in case we decided to revert and apply this PR or post-facto merging this so at least you'd be in the history - weren't sure yet how to handle, sorry about that.

@cwelch5
Copy link
Contributor

cwelch5 commented Jun 20, 2015

Sorry for the confusion, @snow01. The stability of the library was in jeopardy and we were ironing out some details with the CLA. We'd be happy to incorporate your changes so you can get your name credited to the history, if you'd like to contact @potench and iron out the simple CLA. Thanks for your understanding. We hope to avoid this issue going forward.

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.

None yet

4 participants