Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

Conversation

steveschmitt
Copy link
Contributor

Replace document.createElement('a') with new URL();
This allows running on non-browser platforms, e.g. Angular Universal.

Note that URL needs to be shimmed when running on IE or on node < 7, e.g.:

require('isomorphic-url').shim();

Yes, I know it seems strange to run a fake backend on a real backend, but it helps testing.

Copy link
Contributor

@wardbell wardbell left a comment

Choose a reason for hiding this comment

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

See my concern about URL

return l;
// default the base url when not running in browser
let base = document ? document.location.href : 'http://node';
return new URL(href, base);
Copy link
Contributor

Choose a reason for hiding this comment

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

@steveschmitt I'm worried that this won't work in all the browsers we support because the URL type is experimental. I'm not sure it's shimmed.

Would you please check in IE, Edge, and FF. Or come up with an alternative for producing the string we need. I don't think our use of this location stuff needs to be that complicated. I think we can do it with regEx for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works in Chrome, Firefox, and Edge, but fails in IE 11. I'll work on an alternative.

@steveschmitt
Copy link
Contributor Author

Changed to a regex-based approach, without changing the dependent code.

@wardbell wardbell merged commit 0269b0a into angular:master Mar 9, 2017
@wardbell wardbell changed the title fix: can't use document in node feat: make it work in node for Angular universal demos Mar 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants