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

Cannot read property x-ua-compatible of null #1182

Merged
merged 1 commit into from Sep 28, 2014
Merged

Cannot read property x-ua-compatible of null #1182

merged 1 commit into from Sep 28, 2014

Conversation

nathanfaucett
Copy link
Contributor

node v0.13.0-pre url.parse query returns null if no query string, and getXUACompatibleUrl and getXUACompatibleMetaElement are not checking for this

@floverdevel
Copy link

up 👍
i experienced the same bug this week, and after debug session i came up with the exact same fix
then, i search the web to see if i was the only one to have this bug (and why it occurs) and found this pull request :)
is there any chance that it will be merge in a nearly future ?

or maybe this should be fix in nodejs repo ?

@maksimr
Copy link
Contributor

maksimr commented Sep 18, 2014

Hi! Thanks for PR.

Try reproduce on node 0.13. Before node returned empty object if no query string and second argument was true. --Seems we should also open issue in node to clarify default behavior--

@@ -37,7 +37,7 @@ var filePathToUrlPath = function(filePath, basePath) {
var getXUACompatibleMetaElement = function(url) {
var tag = '';
var urlObj = urlparse(url, true);
if (urlObj.query['x-ua-compatible']) {
if (urlObj.query && urlObj.query['x-ua-compatible']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove duplicate check in two functions(Later if they will return empty object then we change only one place) decorated urlparse:

var url = require('url');
var urlparse = function(url){
  var urlObj = url.parse(url, true);
  urlObj.query = urlObj.query || {};
  return urlObj;
};

Then our code will look:

var urlObj = urlparse(url);
if (urlObj.query['x-ua-compatible']) {
...
}

Choose a reason for hiding this comment

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

yep, i think it makes more sens to fix/patch the urlparse function instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will fail, url in the function will be the passed string now

var url = require('url');
var urlparse = function(url){
  var urlObj = url.parse(url, true);
  urlObj.query = urlObj.query || {};
  return urlObj;
};

ill add this to my fork and commit the changes

var url = require('url');
var urlparse = function(urlStr) {
    var urlObj = url.parse(urlStr, true);
    urlObj.query = urlObj.query || {};
    return urlObj;
}

@maksimr
Copy link
Contributor

maksimr commented Sep 18, 2014

LGTM

Forgot ... Can you:

  1. Squash commits to one
  2. Make commit message follow convention

Thanks! 👍

@kingcody
Copy link

Just for documentation purposes; I experience the issue on node 0.11.14 but not on 0.11.13

@mgol
Copy link
Contributor

mgol commented Sep 25, 2014

I confirm: doesn't break 0.11.13, breaks 0.11.14.

Probably way more people will experience this issue now that 0.11.14 has arrived.

@kingcody
Copy link

@mzgol very true, It just showed up in my Travis builds.

maksimr added a commit that referenced this pull request Sep 28, 2014
Cannot read property x-ua-compatible of null
@maksimr maksimr merged commit f72d08f into karma-runner:master Sep 28, 2014
spoike added a commit to reflux/refluxjs that referenced this pull request Sep 29, 2014
Apparently karma currently does not work in latest version of Node. Read
more here:

karma-runner/karma#1182

Fixes #90
Xiphe added a commit to Jimdo/angular-fontselect that referenced this pull request Sep 30, 2014
As a bugfix until karma-runner/karma#1182
is released with karma v0.12.14.
DirectXMan12 added a commit to novnc/noVNC that referenced this pull request Sep 30, 2014
The latest version of Node.js has a bug that
affects the Karma test runner.  A patch has been
merged to Karma, but has not landed in a version
yet.  Until a new version of Karma is released,
we should keep node at 0.11.13.

See karma-runner/karma#1182

(cherry picked from commit 9af2346a0cead634f3af5f390770ea65929c1f4a)
mgol added a commit to jquery/sizzle that referenced this pull request Oct 11, 2014
The previous version is incompatible with Node 0.11.14, see:
karma-runner/karma#1182
mgol added a commit to mgol/angular.js that referenced this pull request Oct 11, 2014
Some of previous dependencies versions (e.g. Karma) didn't work with
Node 0.11.14, see:
karma-runner/karma#1182

The only dependency not updated in this commit is grunt-jscs-checker; its
rules have changed a lot so it will require more work to use the newer
version.
mgol added a commit to mgol/angular.js that referenced this pull request Oct 11, 2014
Some of previous dependencies versions (e.g. Karma) didn't work with
Node 0.11.14, see:
karma-runner/karma#1182

The only dependency not updated in this commit are:

1. grunt-jscs-checker: its
rules have changed a lot so it will require more work to use the newer
version
2. gulp-jshint: the update breaks docs linting, it requires investigation
mgol added a commit to mgol/angular.js that referenced this pull request Oct 11, 2014
Some of previous dependencies versions (e.g. Karma) didn't work with
Node 0.11.14, see:
karma-runner/karma#1182

The only dependency not updated in this commit are:

1. grunt-jscs-checker: its
rules have changed a lot so it will require more work to use the newer
version
2. gulp-jshint: the update breaks docs linting, it requires investigation

Closes angular#9571
mgol added a commit to mgol/angular.js that referenced this pull request Oct 11, 2014
Some of previous dependencies versions (e.g. Karma) didn't work with
Node 0.11.14, see:
karma-runner/karma#1182

The only dependencies not updated in this commit are:

1. grunt-jscs-checker: its
rules have changed a lot so it will require more work to use the newer
version
2. gulp-jshint: the update breaks docs linting, it requires investigation

Closes angular#9571
mgol added a commit to mgol/angular.js that referenced this pull request Oct 13, 2014
Some of previous dependencies versions (e.g. Karma) didn't work with
Node 0.11.14, see:
karma-runner/karma#1182

The only dependencies not updated in this commit are:

1. grunt-jscs-checker: its
rules have changed a lot so it will require more work to use the newer
version
2. gulp-jshint: the update breaks docs linting, it requires investigation

Closes angular#9571
mgol added a commit to angular/angular.js that referenced this pull request Oct 13, 2014
Some of previous dependencies versions (e.g. Karma) didn't work with
Node 0.11.14, see:
karma-runner/karma#1182

The only dependencies not updated in this commit are:

1. grunt-jscs-checker: its
rules have changed a lot so it will require more work to use the newer
version
2. gulp-jshint: the update breaks docs linting, it requires investigation

Closes #9571
openstack-gerrit pushed a commit to openstack-archive/deb-novnc that referenced this pull request Sep 16, 2016
The latest version of Node.js has a bug that
affects the Karma test runner.  A patch has been
merged to Karma, but has not landed in a version
yet.  Until a new version of Karma is released,
we should keep node at 0.11.13.

See karma-runner/karma#1182

(cherry picked from commit 9af2346a0cead634f3af5f390770ea65929c1f4a)
chinnkarahoi pushed a commit to chinnkarahoi/noVNC that referenced this pull request Jan 27, 2023
The latest version of Node.js has a bug that
affects the Karma test runner.  A patch has been
merged to Karma, but has not landed in a version
yet.  Until a new version of Karma is released,
we should keep node at 0.11.13.

See karma-runner/karma#1182

(cherry picked from commit 9af2346a0cead634f3af5f390770ea65929c1f4a)
kingpig32 added a commit to kingpig32/refluxjs that referenced this pull request Apr 5, 2024
Apparently karma currently does not work in latest version of Node. Read
more here:

karma-runner/karma#1182

Fixes #90
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

5 participants