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

Use native Node URL parsing after Node 7.2.1 errors resolved #1186

Closed
ebidel opened this issue Dec 20, 2016 · 9 comments · Fixed by #4712
Closed

Use native Node URL parsing after Node 7.2.1 errors resolved #1186

ebidel opened this issue Dec 20, 2016 · 9 comments · Fixed by #4712

Comments

@ebidel
Copy link
Contributor

ebidel commented Dec 20, 2016

I'm getting URL parsing errors in audits/dobetterweb/uses-http2.js that weren't present before #997.

When running:

npm run fast -- --output=html --output-path=results.html http://www.univision.com

Think it's failing on http://5321212.fls.doubleclick.net/activityi;src=5321212;type=unvsn_un;cat=unvsn_uv;ord=7762287885264.98?

screen shot 2016-12-20 at 2 52 39 pm

@ebidel
Copy link
Contributor Author

ebidel commented Dec 20, 2016

@wardpeet you mind investigating?

Also happening on https://www.healthcare.gov. The url that it is upset about is https://166688199.log.optimizely.com/event?a=166688199&d=166688199&y=true&src=js&s171652904=false&s171674651=none&s171946972=gc&s172159083=direct&s2417020537=true&tsent=1482274480.426&n=https%3A%2F%2Fwww.healthcare.gov%2F&u=oeu1482274480406r0.6877701290450859&wxhr=true&time=1482274480.425&f=7763110370,8016731066,7884071307,8073890089,7980150122,7763860875,7767800396,4681340685,7162490510,7975554292,7989202617,7768580538,8072450782&g=2177170222,2435330307&cx2=95f459d8

@Janpot
Copy link
Contributor

Janpot commented Dec 20, 2016

Coincidentally I ran into this issue too a few days ago: nodejs/node#10306
a fix is on the way but updating the tests revealed more issues: nodejs/node#10317

@ebidel
Copy link
Contributor Author

ebidel commented Dec 20, 2016

Thanks @Janpot. Looks like a Node issue. I'm also using 7.2.1

@ebidel ebidel changed the title URL parsing errors Node 7.2.1 URL parsing errors Dec 20, 2016
@wardpeet
Copy link
Collaborator

wardpeet commented Dec 21, 2016

@ebidel I could use whatwg instead of url for node 7 until it is fixed
Ah looks like @brendankenny already did this :)

@wardpeet
Copy link
Collaborator

closing this one, fixed by #1187

@brendankenny
Copy link
Member

I actually wanted to leave this open since we want to turn back on native node URL parsing when they fix their code. Changing the issue title to match that.

@brendankenny brendankenny reopened this Dec 21, 2016
@brendankenny brendankenny changed the title Node 7.2.1 URL parsing errors Use native Node URL parsing after Node 7.2.1 errors resolved Dec 21, 2016
@paulirish
Copy link
Member

k. The compliance issues that Jan pointed out earlier have landed in v7.4.0, which shipped yesterday.

Shall we add a "Compat:" comment indicating we can retire the whatwg-url module when < 7.4.0 is unsupported?
It will be many months, so I'd prefer that as our TODO rather than keeping this issue open. :)

@wardpeet
Copy link
Collaborator

wardpeet commented Jan 5, 2017

good for me! :)
we might even do a version check in the file to use url when node > 7.4

@brendankenny
Copy link
Member

Should be obsolete by Node 8 reaching LTS

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

Successfully merging a pull request may close this issue.

5 participants