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

refactor(core): include_js_files! 'dir' option doesn't change specifiers #18019

Merged

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Mar 5, 2023

This commit changes "include_js_files!" macro from "deno_core"
in a way that "dir" option doesn't cause specifiers to be rewritten
to include it.

Example:

include_js_files! {
  dir "js",
  "hello.js",
}

The above definition required embedders to use:
import ... from "internal:<ext_name>/js/hello.js".
But with this change, the "js" directory in which the files are stored
is an implementation detail, which for embedders results in:
import ... from "internal:<ext_name>/hello.js".

The directory the files are stored in, is an implementation detail and
in some cases might result in a significant size difference for the
snapshot. As an example, in "deno_node" extension, we store the
source code in "polyfills" directory; which resulted in each specifier
to look like "internal:deno_node/polyfills/<module_name>", but with
this change it's "internal:deno_node/<module_name>".

Given that "deno_node" has over 100 files, many of them having
several import specifiers to the same extension, this change removes
10 characters from each import specifier.

This commit changes "include_js_files!" macro from "deno_core" in a way
that "dir" option doesn't cause specifiers to be rewritten to include it.

Example:
```
include_js_files! {
  dir "js",
  "hello.js",
}
```

The above definition required embedders to use:
`import ... from "internal:<ext_name>/js/hello.js"`.
But with this change, the "js" directory in which the files
are stored is an implementation detail, which for embedders results in:
`import ... from "internal:<ext_name>/hello.js"`.

The directory the files are stored in, is an implementation detail and
in some cases might result in a significant size difference for the snapshot.
As an example, in "deno_node" extension, we store the source code in "polyfills"
directory; which resulted in each specifier to look like
"internal:deno_node/polyfills/<module_name>", but with this change it's
"internal:deno_node/<module_name>". Given that "deno_node" has over 100 files,
many of them having several import specifiers to the same extension, this change
removes 10 characters from each import specifier.
@bartlomieju bartlomieju enabled auto-merge (squash) March 5, 2023 01:54
Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM, great improvement

@bartlomieju bartlomieju merged commit b40086f into denoland:main Mar 5, 2023
@bartlomieju bartlomieju deleted the core_include_js_files_dir_internal branch March 5, 2023 02:31
kt3k pushed a commit that referenced this pull request Mar 10, 2023
…ers (#18019)

This commit changes "include_js_files!" macro from "deno_core"
in a way that "dir" option doesn't cause specifiers to be rewritten 
to include it.

Example:
```
include_js_files! {
  dir "js",
  "hello.js",
}
```

The above definition required embedders to use:
`import ... from "internal:<ext_name>/js/hello.js"`. 
But with this change, the "js" directory in which the files are stored
is an implementation detail, which for embedders results in: 
`import ... from "internal:<ext_name>/hello.js"`.

The directory the files are stored in, is an implementation detail and 
in some cases might result in a significant size difference for the 
snapshot. As an example, in "deno_node" extension, we store the 
source code in "polyfills" directory; which resulted in each specifier 
to look like "internal:deno_node/polyfills/<module_name>", but with 
this change it's "internal:deno_node/<module_name>". 

Given that "deno_node" has over 100 files, many of them having 
several import specifiers to the same extension, this change removes
10 characters from each import specifier.
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