Skip to content

Add import.meta.main#1835

Merged
ry merged 9 commits into
denoland:masterfrom
bartlomieju:meta_main
Feb 26, 2019
Merged

Add import.meta.main#1835
ry merged 9 commits into
denoland:masterfrom
bartlomieju:meta_main

Conversation

@bartlomieju

@bartlomieju bartlomieju commented Feb 24, 2019

Copy link
Copy Markdown
Member

Closes #1834

I tried to move ImportMeta declaration to separate, however TS was crushing then, I'll investigate that tomorrow.

I had also test in C++, but it felt duplicated with TS test.

@kitsonk

kitsonk commented Feb 24, 2019

Copy link
Copy Markdown
Contributor

As this stands at the moment, it won't flow through to the runtime type library. (If you do deno --types you will see it is missing).

The interface is already a global interface (interface ImportMeta {}) as it has been reserved in the language for a long time. We need to extend this interface in the global scope, which is what happens when it is located in lib.web_assembly.d.ts file as that is simply inlined in the runtime type library, but that isn't the "right" way to do it for extending this in the long term. I think it is better that we put this directly in the globals.ts, the only problem is that we need to change ts_library_builder in the mergeGlobal function to copy over any interfaces there into the type library.

That is a bit complicated, so if you wanted to, just move it back to the web assembly library, and then I can do a follow up PR making the change to ts_library_builder. Otherwise you can try to tackle it. It should be added somewhere around here and needs to be done is iterate over any interfaces, ensuring it weeds out the special global interface and adds those to the target.

Comment thread libdeno/binding.cc Outdated
@bartlomieju

bartlomieju commented Feb 25, 2019

Copy link
Copy Markdown
Member Author

@kitsonk actually I get:

$ deno --types | grep -i importmeta -A 3 -B 3
// @url js/lib.import_meta.d.ts

interface ImportMeta {
  url: string;
  main: boolean;
}

EDIT: Okay I reread your comment and understand the problem. Let's tackle it in another PR.

@ry

ry commented Feb 25, 2019

Copy link
Copy Markdown
Member

Maybe we can add an argument to deno_mod_new() to tell if it's main...

@bartlomieju

bartlomieju commented Feb 25, 2019

Copy link
Copy Markdown
Member Author

@ry I can then place it in ModuleInfo - ModuleInfo.main?

@ry

ry commented Feb 25, 2019

Copy link
Copy Markdown
Member

@bartlomieju yes that makes sense to me

@bartlomieju

Copy link
Copy Markdown
Member Author

@ry not super happy because there's a lot of bool flags passing but it got job done

Comment thread BUILD.gn Outdated
Comment thread libdeno/modules_test.cc Outdated
Comment thread libdeno/modules_test.cc Outdated
Comment thread tests/import_meta.ts.out

@ry ry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work! Just a few comments.

@bartlomieju

Copy link
Copy Markdown
Member Author

@ry changes applied

Comment thread js/lib.web_assembly.d.ts
Comment thread libdeno/modules_test.cc
deno_mod_evaluate(d, d, throw_main);
EXPECT_EQ(nullptr, deno_last_exception(d));

deno_delete(d);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice test!

@bartlomieju

Copy link
Copy Markdown
Member Author

Thanks @ry, somehow missed that file...

@ry ry left a comment

Copy link
Copy Markdown
Member

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 5dfbbbb into denoland:master Feb 26, 2019
@bartlomieju
bartlomieju deleted the meta_main branch February 26, 2019 18:31
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