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

Remove `typeof exports` check in Hogan libraries #8

Merged
merged 2 commits into from May 18, 2015

Conversation

Projects
None yet
3 participants
@fofr
Contributor

fofr commented May 18, 2015

Background

CommonJS uses a global exports variable to export properties from a module. A pattern in frontend libraries is to check for the presence of exports to see if the library, in its current context, should behave as a module:

(typeof exports !== 'undefined' ? exports : Hogan)

The problem

Sometimes this exports global variable is already present. In our case it’s by a quirk of dynamically generated heading IDs which then become global variables. Browsers, according to the HTML5 spec, create global variables for every element with an ID attribute, standardising an old IE behaviour.

An example

## Exports in markdown becomes <h2 id="exports">Exports</h2>. Browsers take the ID and create window.exports for "named access on the window object". Checking typeof exports !== 'undefined' gives a false positive, and is no longer a valid context check. This in turn leads to an HTML element being passed to the Hogan script, rather than the vanilla object it expects. Subsequent calls to new Hogan.Template then fail.

A live example:
https://www.gov.uk/government/publications/notice-143-a-guide-for-international-post-users/notice-143-a-guide-for-international-post-users

The fix

  • The exports check can be removed as we aren’t using the commonjs module pattern.
  • This is a hotfix to a vendor asset, and should ideally be fixed upstream (possibly with a check that document.getElementById('exports') doesn't exist).
  • The file has been renamed to make the hotfix clear

Fixes https://www.pivotaltracker.com/story/show/87097366

cc @edds @tombye

fofr added some commits May 18, 2015

Remove `typeof exports` check
Sometimes the `exports` global variable is present through a quirk of
dynamically generated heading IDs which the browser makes accessible
through the window namespace. eg <h2 id="exports">Exports</h2> becomes
`window.exports` and typeof exports is no longer undefined.

Creating a global variable for each element with an ID is a defined
HTML5 behaviour:
http://www.w3.org/html/wg/drafts/html/master/browsers.html#named-access-
on-the-window-object

The exports check can be removed altogether as we aren’t making use of
the commonjs module pattern.

This is a hotfix to a vendor asset, and should ideally be fixed
upstream.
Rename Hogan files to indicate hotfix
Make it obvious that these aren’t the original vendor files.
@edds

This comment has been minimized.

Contributor

edds commented May 18, 2015

👍

edds added a commit that referenced this pull request May 18, 2015

Merge pull request #8 from alphagov/exports-defined
Remove `typeof exports` check in Hogan libraries

@edds edds merged commit 17fb241 into master May 18, 2015

@edds edds deleted the exports-defined branch May 18, 2015

@dsingleton

This comment has been minimized.

dsingleton commented May 18, 2015

👏

fofr added a commit that referenced this pull request May 18, 2015

Version 0.2.1.
Remove `typeof exports` check in Hogan libraries:
#8

@fofr fofr referenced this pull request May 18, 2015

Merged

Update references to Hogan file #9

fofr added a commit that referenced this pull request May 19, 2015

Version 0.2.1.
Remove `typeof exports` check in Hogan libraries:
#8

fofr added a commit that referenced this pull request May 19, 2015

Version 0.2.1
Remove `typeof exports` check in Hogan libraries:
#8
#9

This was referenced May 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment