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 url-parse package as parsing fallback #267

Closed
wants to merge 1 commit into from

Conversation

FLGMwt
Copy link

@FLGMwt FLGMwt commented Sep 4, 2019

Same as #219, hoping to get pretender working with React Native projects that don't have document.createElement available for URL parsing. url-parse has a minimal minimal footprint and this won't affect other consumers. That said, I'm happy to rework it to not use document.createElement at all and rely only on url-parse for consistency.

As for the self reference that #219 offered, I've found that it's common for RN projects using browser-y libraries to use react-native-browser-polyfill to achieve something similar, so changes aren't necessary here. Otherwise, I can pull in the suggestion provided in #219 if that's preferred.

Thanks : D

@samselikoff
Copy link
Contributor

👍 I'm in favor of this change – and I also think we should drop the try/catch and just use url-parse! The lib is popular & well-maintained, don't think there's any reason to preserve the original behavior.

@FLGMwt FLGMwt force-pushed the use-url-parse-package branch 2 times, most recently from b21ba0e to b9473c3 Compare September 13, 2019 16:37
@FLGMwt
Copy link
Author

FLGMwt commented Sep 13, 2019

Mk, updated to use url-parse unconditionally and yarn test browser-based tests are fine, but yarn test-ci in Phantom is sad.

Any ideas? Am I missing something needed for including the url-parse package?

EDIT:

D'oh URL is a browser global 🤦‍♀️

@samselikoff
Copy link
Contributor

Ah. Maybe you can just map it and call it something else?

@FLGMwt
Copy link
Author

FLGMwt commented Sep 13, 2019

Ah. Maybe you can just map it and call it something else?

Yeah, found another issue after that though w/ the new name not being available to the iife. Couldn't figure out how the other deps were working since I was doing the same thing as them. My best guess is that whatwg and the two bower deps had umd builds that exposed themselves on global but url-parse doesn't. Fix here is to tell rollup not to mark it as a external/already dealt-with dep so it gets included in the rollup. I think? 🤷‍♂

EDIT

why are the close and comment buttons so close to each other

@FLGMwt FLGMwt closed this Sep 13, 2019
@FLGMwt FLGMwt reopened this Sep 13, 2019
@FLGMwt FLGMwt force-pushed the use-url-parse-package branch 2 times, most recently from e88e03a to c2743bf Compare September 13, 2019 19:31
@samselikoff
Copy link
Contributor

LGTM!

I think we don't need to include url-parse in the es build but in master all the config is shared. We're doing work on the Rollup config in our PR so I vote we merge this and then we can improve the es build either in our PR or a separate one. Keep it small ya know.

Copy link
Member

@xg-wang xg-wang left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

If we are going to include a polyfill for parsing url (probably only for IE), an alternative is to include core-js's URL polyfill. I did a size measurement with terser minified file:

  1. original pretender:
./dist/pretender.js 18.44 KB
./dist/pretender.es.js 16.26 KB
./dist/pretender.es.min.js 6.68 KB
  1. url-parse
./dist/pretender.js 34.02 KB
./dist/pretender.es.js 31.77 KB
./dist/pretender.es.min.js 10.97 KB
  1. import 'core-js/web/url';
./dist/pretender.js 103.7 KB
./dist/pretender.es.js 97.09 KB
./dist/pretender.es.min.js 34.1 KB

url-parse seems to be a more reasonable alternative if core-js polyfill is too large. Otherwise I would prefer to polyfill with core-js so we can use standard URL.

@FLGMwt @samselikoff @stefanpenner Is size something you worry about? My use of Pretender is always in test so I don't actually care about the bloating size.

@@ -53,6 +53,7 @@
"dependencies": {
"fake-xml-http-request": "^2.0.0",
"route-recognizer": "^0.3.3",
"url-parse": "^1.4.7",
Copy link
Member

Choose a reason for hiding this comment

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

To bundle url-parse in the output file, url-parse should lives in devDependencies.

@@ -6,15 +6,16 @@ const fs = require('fs');
const globals = {
'whatwg-fetch': 'FakeFetch',
'fake-xml-http-request': 'FakeXMLHttpRequest',
'route-recognizer': 'RouteRecognizer'
'route-recognizer': 'RouteRecognizer',
'url-parse': 'ParsedUrl'
Copy link
Member

Choose a reason for hiding this comment

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

The global in output file seems to be urlParse rather than ParsedUrl.

module.exports = {
input: 'src/index.ts',
external: Object.keys(pkg.dependencies),
// don't exclude url-parse because it does *not* provide a UMD bundle and needs to be included
external: Object.keys(pkg.dependencies).filter(x => x !== 'url-parse'),
Copy link
Member

Choose a reason for hiding this comment

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

This need to be changed when updating how this package is being bundled.

// eslint-disable-next-line no-self-assign
anchor.href = anchor.href; // IE: load the host and protocol
parsedUrl.href = parsedUrl.href; // IE: load the host and protocol
Copy link
Member

Choose a reason for hiding this comment

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

I believe the comment was for IE quirks. This should be changed when moving away from anchor tag method.

host = anchor.hostname; // IE: remove default port
var host = parsedUrl.host;
if (parsedUrl.port === '80' || parsedUrl.port === '443') {
host = parsedUrl.hostname; // IE: remove default port
Copy link
Member

Choose a reason for hiding this comment

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

Same as my comment above. These seem to be addressing IE issue, we should have test coverage on the mentioned cases when moving to url-parse.
Although there's no cross browser setup yet, but it's still worth adding test for this.

@samselikoff
Copy link
Contributor

If we are going to include a polyfill for parsing url (probably only for IE), an alternative is to include core-js's URL polyfill

@xg-wang The reason we're bringing in url-parse is not to polyfill old browsers but to have better cross-environment support, so Pretender can be used in JS environments that don't have document. (In this case, that was React Native, but you could imagine wanting to use Pretender in a non-browser node environment too.)

If consumers need to polyfill ie11 behavior they should be able to use core-js on their own (and have it apply to not only Pretender but all their dependencies...)

What do you think?

@samselikoff
Copy link
Contributor

And I agree it would be nice to get test coverage for ie11. But not exactly sure how. (I guess ie11 test coverage is more important for the iife build as folks using the es build will have a build step and should be able to use core-js on their own.)

@xg-wang
Copy link
Member

xg-wang commented Sep 14, 2019

URL constructor is available since node 7 https://nodejs.org/api/url.html#url_the_whatwg_url_api

@samselikoff
Copy link
Contributor

Oh I see – you're saying this PR could use the URL class instead of document.createElement or url-parse?

@xg-wang
Copy link
Member

xg-wang commented Sep 14, 2019

Oh sorry I'm not familiar with React Native, seems it's not implemented facebook/react-native#16434, and the WIP implementation just throws error https://github.com/facebook/react-native/blob/v0.60.5/Libraries/Blob/URL.js#L111.

I think using url-parse is fine here, parsing is the only functionality pretender is using.

@samselikoff
Copy link
Contributor

Gotcha 👍

Ok so where does that leave us? (1) remove the ie11 comments (2) and what should we do about core js/ ie11? Are you wanting to ensure the iife build works in ie11, or that an Ember app does that's using the es build?

@xg-wang
Copy link
Member

xg-wang commented Sep 14, 2019

@samselikoff https://github.com/unshiftio/url-parse#url-parse has a good test coverage. I'm thinking about bundling and versioning.

Right now pretender is importing fake-xml-http-request, route-recognizer, and optional whatwg-fetch. Should url-parse also be treated as external dependency and let consumer import the package? The 3 existing packages all have UMD and ES module distribution but url-parse doesn't.

I think what would be easier to consume pretender is to export

  • UMD module dist/pretender.js with bundled required packages, that is without whatwg-fetch.
  • ES module dist/pretender.es.js is the ES module version of the UMD module.
  • UMD module dist/pretender.full.umd.js with all dependencies.

I have a spike at xg-wang@0c17e6e
Build output can be found at xg-wang@e7124ef

@trek @stefanpenner what's your opinion on bundling? If this approach looks good to you I can create a PR, and @FLGMwt can just address my comments and this PR should be good to go.

@samselikoff
Copy link
Contributor

@xg-wang perhaps we can pick this up over in #269? We started work over there.

Not sure if we need to switch from iife to umd. First question would be, who's using the iife? Supporting all these builds gets complex. I'm leaning more and more these days to focusing on esm build.

The instructions on README for Pretender have said

You can load Pretender directly in the browser.

<script src="pretender.js"></script>

but I'm pretty sure that's wrong, I believe the other dependencies would also need to be loaded, leading me to think nobody is using this, further leading me to think we should not commit to supporting a new UMD build (and definitely not two). It's just more work and the use cases get very complex very fast.

In #269 we preserve the existing iife build and also swap whatwg-fetch out for cross-fetch during the ESM build, since whatwg-fetch cannot be imported in node.

Also looking at your commit, I don't think we want to treat fake-xml-http-request or route-recognizer as internal to Pretender, we want these to be import statements in the final ESM build so that consumers can bring their own and/or their build tools can dedup them if they happen to be using these deps elsehwere.

@samselikoff
Copy link
Contributor

@xg-wang want to chat about this in Discord, so we don't duplicate any more work?

@samselikoff
Copy link
Contributor

@xg-wang would be great if we get this merged, I don't think we should block @FLGMwt's work on a plan to re-work the rollup config & build system.

What do you think? What work if any is still needed here?

@xg-wang
Copy link
Member

xg-wang commented Sep 25, 2019

@samselikoff I think bundling the package is fine, the remaining work should be to address my comments above. @FLGMwt can you take a look?

@zomars
Copy link

zomars commented Jan 24, 2020

Can I pick up @FLGMwt 's work from here?

@xg-wang
Copy link
Member

xg-wang commented Feb 6, 2020

Thank you @FLGMwt I think this is closed by #288

@xg-wang xg-wang closed this Feb 6, 2020
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.

4 participants