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

Remove 'deno' builtin module #1895

Merged
merged 2 commits into from
Mar 8, 2019
Merged

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Mar 7, 2019

BREAKING CHANGE

This PR removes the builtin module "deno". Only the use of the namespace of Deno is supported.

@ry
Copy link
Member

ry commented Mar 7, 2019

@kitsonk I'm still seeing Deno stuff twice in --types

> ./target/debug/deno --types | grep noColor
  export let noColor: boolean;
  export let noColor: boolean;

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 7, 2019

@ry Oops. Yeah. That was the other change I forgot. Will fix in a little bit.

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 7, 2019

@ry the build failure (https://travis-ci.com/denoland/deno/jobs/183238342#L1796) looks like some sort of intermittent CI stability issue versus a problem with this patch.

@ry
Copy link
Member

ry commented Mar 8, 2019

@kitsonk from just above that in the build log -

Uncaught NotFound: Cannot resolve module "deno" from "/home/travis/build/denoland/deno/js/deps/https/deno.land/std/http/http_bench.ts"
    at DenoError (deno/js/errors.ts:22:5)
    at maybeError (deno/js/errors.ts:41:12)
    at maybeThrowError (deno/js/errors.ts:29:15)
    at sendSync (deno/js/dispatch.ts:67:5)
    at fetchModuleMetaData (deno/js/os.ts:80:19)
    at _resolveModule (deno/js/compiler.ts:250:38)
    at resolveModuleNames (deno/js/compiler.ts:480:35)
    at compilerHost.resolveModuleNames (deno/third_party/node_modules/typescript/lib/typescript.js:117517:138)
    at resolveModuleNamesWorker (deno/third_party/node_modules/typescript/lib/typescript.js:86226:127)
    at resolveModuleNamesReusingOldState (deno/third_party/node_modules/typescript/lib/typescript.js:86451:24)

I guess we need to land denoland/std#247 and upgrade std in deno. complete

This was referenced Mar 8, 2019
@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 8, 2019

cool... all clear now

@ry
Copy link
Member

ry commented Mar 8, 2019

@kitsonk PTAL I've added a commit to remove the builtin logic from libdeno as well

@kitsonk
Copy link
Contributor Author

kitsonk commented Mar 8, 2019

@ry LGTM... I thought about doing that as well, but thought it was an ok feature that might be needed in the future, but that really seems unlikely, as if we decided the Deno namespace became too much to manage we would simply add other namespaces and if the use case did ever come up it could be added back in.

@ry
Copy link
Member

ry commented Mar 8, 2019

@kitsonk I hesitated too - but I think we should remove now because

  1. it's dead code, and dead code should be removed.
  2. it's very complex and poorly implemented.
  3. it makes the module system simpler - which is generally a good thing
    Glad to be rid of this baggage !

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 24d6bf6 into denoland:master Mar 8, 2019
hashrock pushed a commit to hashrock/deno-opn that referenced this pull request May 23, 2019
There has been a breaking API change in Deno v.0.3.3 that also breaks
this module. The import of "deno" module got replaced with a global Deno
variable.

The "deno" module removal PR: denoland/deno#1895
Release notes of v.0.3.3: denoland/deno@3dbb06e#diff-30ebd105aef11de730b7b473a247e99bR16
@kitsonk kitsonk deleted the remove-deno-module branch August 2, 2022 04:44
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.

2 participants