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

Enable real browser test #146

Closed
wants to merge 12 commits into from
Closed

Enable real browser test #146

wants to merge 12 commits into from

Conversation

loynoir
Copy link

@loynoir loynoir commented Aug 7, 2021

Users

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

  1. git clone then visit browser-test/index.html
  2. 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
Copy link
Author

loynoir commented Aug 7, 2021

#105
#142
#140

1. Undo editor auto format to make test happy
2. temporary disable prefer-regex-literals
1. [test] Move `ava` to `mocha+chai` to test in real browser.
(sindresorhus#105) (sindresorhus#140) (sindresorhus#142)
2. [test] seprate non special-protocol-schemes(sindresorhus#147)
3. [doc] Update readme
@loynoir loynoir changed the title Add fallback when not support js-regexp-lookbehind Enable real browser test Aug 9, 2021
1.[tests] Allow less strict invalid URL check
2.[tests] Provide useful msg on left to get feedback from userland
Seperate slash test add after v4, to identify problems.
1. use reverse + js-RegExp-Lookahead + reverse as fallback to js-RegExp-lookbehind
2. 2 runtime logics times, two period of test, 4 situations are tested, fully backward compact
…hind)` 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.
@sindresorhus
Copy link
Owner

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
Copy link
Author

loynoir commented Aug 11, 2021

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
Copy link
Author

loynoir commented Aug 11, 2021

FYI, there are something I found so far.

  1. options validate should ASAP

    if (options.forceHttps && urlObject.protocol === 'http:') {

  2. In modern and old browser, custom protocol error like issue#147
    Fixed in my latest fork

  3. In old browser, name capture group may break ok code like issues#105

    const match = /^data:(?<type>[^,]*?),(?<data>[^#]*?)(?:#(?<hash>.*))?$/.exec(urlString);

  4. In old browser, there is no URLSearchParams.prototype.sort

Have a nice day @sindresorhus 😊

@sindresorhus
Copy link
Owner

sindresorhus commented Aug 11, 2021

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
Copy link
Owner

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

#132

@sindresorhus
Copy link
Owner

options validate should ASAP

Not sure what you mean?

@sindresorhus
Copy link
Owner

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
Copy link
Author

loynoir commented Aug 11, 2021

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
Copy link
Author

loynoir commented Aug 11, 2021

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
Copy link
Author

loynoir commented Aug 11, 2021

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
Copy link
Owner

I'm not interested in browser testing.

@loynoir
Copy link
Author

loynoir commented Aug 11, 2021

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
Copy link
Author

loynoir commented Aug 11, 2021

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?

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

2 participants