Enable real browser test

loynoir commented 3 years ago

Users

There are two ways if you want to test version >= 7.0.0 in real browser,

git clone then visit browser-test/index.html
Zero install using rawgit, link. Remember to change hash to latest commit.

Changes

[source] Change /JsRegexpLookbehind/ to new RegExp(JsRegexpLookbehind), prevent syntax error breaks user code.
[source] Use reverse-lookahead-reverse as fallback when not support JsRegexpLookbehind.
[source] Add URLSearchParameters.sort ponyfill as fallback.
[source] Add options preferJsRegexpLookbehind and preferURLSearchParamsSort to run fallback logic in modern runtime.
[types] Add above two options to .d.ts.
[source] Move normalizeUrl(options) options validation to most top.
[source] Treat custom protocol as .defaultProtocol, to prevent host as part of pathname.
[source] Change back custom protocol after urlObject.toString, to prevent pathname changed again.
[workflow] Add karma to enable automate headless browser tests.
[workflow] Add rollup to transform regexp named-capturing-groups and bundle tests for browser.
[tests] Change ava to mocha+chai, enable to test it in Node.js / Headless Chrome / Headless Firefox / Any real browsr.
[tests] Update prev invalid urls to match more than Node.js env.
[tests] Seperate some long tests in to small parts to easy identify problems.

Test Status

(32/32) Test passed with

Node.js 14
Chrome headless driver
Firefox headless driver
Chrome 92
Firefox 90
Firefox 68 ESR
Opera59
Yandex 7 (2017)

loynoir commented 3 years ago

https://github.com/sindresorhus/normalize-url/issues/105 https://github.com/sindresorhus/normalize-url/issues/142 https://github.com/sindresorhus/normalize-url/issues/140

sindresorhus commented 3 years ago

Safari support was fixed in https://github.com/sindresorhus/normalize-url/releases/tag/v7.0.1. I'm not interested in browser tests, browser-specific options, or the .sort polyfill. You also have way too many unrelated changes in a single pull request.

loynoir commented 3 years ago

Hi, @sindresorhus

I'm not interested in browser tests

But how can you guarantee this library works in browser, without testing it in browser?

Eg: issue#147

browser-specific options

I know that you may not like that option, so I did try to remove it. But test-runtime is modern, need to way to run all logic. So I restore them.

You also have way too many unrelated changes in a single pull request

I'm kind of non professional newbie. Should I combine all commits as one, and break them into small features. Then one feature -> contribute -> one feature -> contribute?

loynoir commented 3 years ago

FYI, there are something I found so far.

options validate should ASAP https://github.com/sindresorhus/normalize-url/blob/24560330a31eda9253172c02339ec610f5917d4b/index.js#L105
In modern and old browser, custom protocol error like issue#147 Fixed in my latest fork
In old browser, name capture group may break ok code like issues#105 https://github.com/sindresorhus/normalize-url/blob/24560330a31eda9253172c02339ec610f5917d4b/index.js#L8
In old browser, there is no URLSearchParams.prototype.sort

Have a nice day @sindresorhus 😊

sindresorhus commented 3 years ago

But how can you guarantee this library works in browser, without testing it in browser?

I never made it for the browser. But it doesn't use anything specific to Node.js, so if it works in Node.js, it should work in modern browsers too.

sindresorhus commented 3 years ago

In modern and old browser, custom protocol error like issue#147

https://github.com/sindresorhus/normalize-url/pull/132

sindresorhus commented 3 years ago

options validate should ASAP

Not sure what you mean?

sindresorhus commented 3 years ago

Old browser support is not a concern for this package. It's up to you to use Babel and polyfill if you need to use it in older browsers. I only merged the Safari fix as it applies to latest Safari.

loynoir commented 3 years ago

In modern and old browser, custom protocol error like issue#147

132

Not sure for that pull, but existing today-test.js failed for both modern and old browser issue#147

loynoir commented 3 years ago

options validate should ASAP

Not sure what you mean?

 function normalizeUrl(urlString, options) {
 // ...code
    if (options.forceHttp && options.forceHttps) {
        throw new Error('The `forceHttp` and `forceHttps` options cannot be used together');
    }
// ...code
}

 function normalizeUrl(urlString, options) {
    if (options.forceHttp && options.forceHttps) {
        throw new Error('The `forceHttp` and `forceHttps` options cannot be used together');
    }
 // ...code
// ...code
}

loynoir commented 3 years ago

But how can you guarantee this library works in browser, without testing it in browser?

I never made it for the browser. But it doesn't use anything specific to Node.js, so if it works in Node.js, it should work in modern browsers too.

But your "this child" seems to be also used in browser from recent issues? Maybe consider using convert ava to mocha+chai+karma? As it also provide automate browser test ability for headless browser drivers?

sindresorhus commented 3 years ago

I'm not interested in browser testing.

loynoir commented 3 years ago

But it doesn't use anything specific to Node.js, so if it works in Node.js, it should work in modern browsers too.

URL implement is different. When custom protocol, node behavior is whatwg-url, but browser is not, A://B/C host in browser is empty.

loynoir commented 3 years ago

I'm not interested in browser testing.

But I saw issues related to browser, may it give a try, decrease future potential issues? As it can be automated?

sindresorhus / normalize-url