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

feat(ssr): auto-remove initial state script if prod [#6761] #6763

Merged

Conversation

markbrouch
Copy link
Contributor

@markbrouch markbrouch commented Oct 10, 2017

Enable intial state script to automatically remove itself from the DOM if server environment is
prod. (#6761)

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Enable intial state script to automatically remove itself from the DOM if server environment is
prod.

vuejs#6761
@markbrouch markbrouch changed the title feat(ssr): auto-remove initial state script if prod feat(ssr): auto-remove initial state script if prod [#6761] Oct 10, 2017
@markbrouch
Copy link
Contributor Author

I could use some help adding tests for this, not sure the best way to go about that.

@@ -191,10 +191,13 @@ export default class TemplateRenderer {
contextKey = 'state',
windowKey = '__INITIAL_STATE__'
} = options || {}
const autoRemove = process.env.NODE_ENV === 'production'
? 'var s;(s=document.currentScript||document.scripts[document.scripts.length-1]).parentNode.removeChild(s);'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

document.currentScript is not supported in IE yet, hence the fallback.

Copy link
Member

Choose a reason for hiding this comment

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

This block should be wrapped in an IIFE to avoid leaking s to global scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 2409ff0

@@ -191,10 +191,13 @@ export default class TemplateRenderer {
contextKey = 'state',
windowKey = '__INITIAL_STATE__'
} = options || {}
const autoRemove = process.env.NODE_ENV === 'production'
? 'var s;(s=document.currentScript||document.scripts[document.scripts.length-1]).parentNode.removeChild(s);'
Copy link
Member

Choose a reason for hiding this comment

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

This block should be wrapped in an IIFE to avoid leaking s to global scope.

Wrap auto-remove script in IIFE so that global scope is not polluted.

vuejs#6761
@yyx990803 yyx990803 merged commit 2d32b5d into vuejs:dev Oct 10, 2017
@markbrouch markbrouch deleted the feature/auto-remove-initial-state-script branch October 10, 2017 15:07
@whxaxes
Copy link

whxaxes commented Oct 13, 2017

it seems miss a semicolon between window.__INITIAL_STATE__={} and autoRemove ?

return context[contextKey]
      ? `<script>window.${windowKey}=${
          serialize(context[contextKey], { isJSON: true })
        }${autoRemove}</script>`
      : ''

it occurs error without semicolon

image

image

@chris-ng-scmp
Copy link

chris-ng-scmp commented Oct 13, 2017

@yyx990803 2.5.0 released 4 hours ago, and I just found the new change will cause syntax error in browser
<script>window.${windowKey}=${ serialize(context[contextKey], { isJSON: true }) }${autoRemove}</script>

in real output will be
window.__INITIAL_STATE__={...}(function(){var s;(s=document.currentScript||document.scripts[document.scripts.length-1]).parentNode.removeChild(s);}())

ztlevi pushed a commit to ztlevi/vue that referenced this pull request Feb 14, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
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.

4 participants