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

provide core internals via import meta #491

Closed
wants to merge 1 commit into from

Conversation

lal12
Copy link
Contributor

@lal12 lal12 commented Apr 16, 2024

The other tjs.internal symbols could be resolved similar.

@lal12 lal12 force-pushed the internals-via-import-meta branch from bd1506e to 11c4e74 Compare April 16, 2024 21:44
@lal12 lal12 marked this pull request as draft April 16, 2024 23:30
@lal12 lal12 force-pushed the internals-via-import-meta branch from 11c4e74 to a0266d7 Compare April 18, 2024 00:09
@lal12 lal12 force-pushed the internals-via-import-meta branch from a0266d7 to 556a93a Compare April 18, 2024 00:19
@saghul
Copy link
Owner

saghul commented Apr 18, 2024

At a glance I'm not very convinced this is an improvement... If anything, it makes it easier to access the core, which is not something desirable, in general.

@lal12
Copy link
Contributor Author

lal12 commented Apr 18, 2024

Why does it make it easier to access the core? Only internal JS should now be able to access the core?

@saghul
Copy link
Owner

saghul commented Apr 18, 2024

Why does it make it easier to access the core?

Because it's accessible to any JS module, without having to jump through the hidden symbol hoop.

Only internal JS should now be able to access the core?

That has always been the case 😅 The public API is the tjs global and the tjs:xxx modules. The core object is a big part of tjs, but there are many things we hide or we add to.

@lal12
Copy link
Contributor Author

lal12 commented Apr 18, 2024

Sorry my reply was a bit misleading. Completly hiding the internals was exactly the goal. As far as I understand it and as far as I tried it, only the internal JS will be able to access import.meta.core, while the user code won't have it (import.meta.core == undefined -> true).

Though maybe I am overlooking something?

@saghul
Copy link
Owner

saghul commented Apr 18, 2024

Hum, is that because you are not adding it to the meta object for non internal modules?

@lal12
Copy link
Contributor Author

lal12 commented Apr 18, 2024

Hum, is that because you are not adding it to the meta object for non internal modules?

Basically. Usually this would also be tricky when internal js imports another internal modue, since import.meta is redefined for every module, but since the internal js is compiled into a single file it is fine.

@lal12
Copy link
Contributor Author

lal12 commented Apr 19, 2024

@saghul just to be clear on the state of this. Do you still looking into it / thinking about it? Or are you waiting for me to finish the draft? Or...

@saghul
Copy link
Owner

saghul commented Apr 19, 2024

TBH I'm not sure. I appreciate the elegance of it, but I worry there is no scape hatch if I need one...

I'd like to leave this in pause a little bit since it has no actual impact and come back to it when something tells us it's either the right thing to pursue or drop.

@saghul
Copy link
Owner

saghul commented Jul 15, 2024

Some time has passed and I am currently happy with the current solution. We can always come back to this PR if we find a good reason to do that.

Thanks for the effort though!

@saghul saghul closed this Jul 15, 2024
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