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 import.meta.main #1835

Merged
merged 9 commits into from
Feb 26, 2019
Merged

Add import.meta.main #1835

merged 9 commits into from
Feb 26, 2019

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Feb 24, 2019

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
Copy link
Contributor

kitsonk commented Feb 24, 2019

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.

libdeno/binding.cc Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

bartlomieju commented Feb 25, 2019

@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
Copy link
Member

ry commented Feb 25, 2019

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

@bartlomieju
Copy link
Member Author

bartlomieju commented Feb 25, 2019

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

@ry
Copy link
Member

ry commented Feb 25, 2019

@bartlomieju yes that makes sense to me

@bartlomieju
Copy link
Member Author

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

BUILD.gn Outdated Show resolved Hide resolved
libdeno/modules_test.cc Outdated Show resolved Hide resolved
libdeno/modules_test.cc Outdated Show resolved Hide resolved
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.

Nice work! Just a few comments.

@bartlomieju
Copy link
Member Author

@ry changes applied

js/lib.web_assembly.d.ts Outdated Show resolved Hide resolved
deno_mod_evaluate(d, d, throw_main);
EXPECT_EQ(nullptr, deno_last_exception(d));

deno_delete(d);
Copy link
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
Member Author

Thanks @ry, somehow missed that file...

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 : )

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.

3 participants