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

Add window.location #1761

Merged
merged 2 commits into from Feb 19, 2019

Conversation

3 participants
@ry
Copy link
Collaborator

ry commented Feb 13, 2019

Fixes #1750

@kitsonk
Copy link
Contributor

kitsonk left a comment

Some thoughts...

Show resolved Hide resolved js/globals.ts Outdated
Show resolved Hide resolved js/globals.ts Outdated
Show resolved Hide resolved js/globals.ts Outdated
Show resolved Hide resolved js/location.ts Outdated
Show resolved Hide resolved js/globals.ts Outdated

@ry ry force-pushed the ry:location branch from 37a023b to 95114d9 Feb 13, 2019

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 13, 2019

LGTM, but I would double check the lib.deno_runtime.d.ts is sane if you haven't

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Feb 13, 2019

--types looks sane. thanks

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Feb 13, 2019

Hmm - maybe I spoke too soon. It seems like there are a few definitions:

> ./target/debug/deno --types | grep "interface Location"
  interface Location {
  export interface Location {
  interface Location {
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Feb 13, 2019

Ya, so it's listed three times inside of

   6 declare namespace Deno {

1589 declare namespace domTypes {

2686 declare module "deno" {

Not clear to me why it's trying to put it in the deno module.

@ry ry force-pushed the ry:location branch 3 times, most recently from 8eea5c1 to 92d4cdd Feb 13, 2019

Show resolved Hide resolved js/location.ts
@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 13, 2019

@ry it isn't this PR that is "leaking" dom_types.ts into Deno. There is another deno.ts module/dependency that is pulling in those types and leaking them, so when it flattens the Deno namespace it pulls in all of the dom_types.ts because it doesn't know how to shake them. I am going to try to figure out which one is doing it.

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 13, 2019

@ry with #1765 the dom_types.ts won't be added to the Deno namespace.

@ry ry force-pushed the ry:location branch 3 times, most recently from 857479e to a036971 Feb 13, 2019

@ry ry referenced this pull request Feb 19, 2019

Merged

v0.3.0 #1807

Show resolved Hide resolved BUILD.gn
Show resolved Hide resolved js/dom_types.ts
Show resolved Hide resolved js/globals.ts Outdated
Show resolved Hide resolved website/manual.md

@ry ry force-pushed the ry:location branch from cb2be97 to 494a795 Feb 19, 2019

@ry ry merged commit c0b8756 into denoland:master Feb 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.