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.

Sign up for GitHub

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

Jump to bottom

Improvements to bundling. #3965

Merged
merged 6 commits into from
Feb 12, 2020
Merged

Improvements to bundling. #3965

merged 6 commits into from
Feb 12, 2020

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Feb 11, 2020

Moves to using a minimal System loader for bundles generated by Deno. TypeScript in 3.8 will be able to output TLA for modules, and the loader is written to take advantage of that as soon as we update Deno to TS 3.8.

System also allows us to support import.meta and provide more ESM aligned assignment of exports, as well as there is better handling of circular imports.

The loader is also very terse versus to try to save overhead.

Also, fixed an issue where abstract classes were not being rexported.

It is worth noting that this cannot be used for the internal snapshot bundles, because it turns the whole instantiation process into an async process which executes out of turn, which causes some problems in setting up the runtime environment. The long term plan though for the snapshots is not to use a "bundle" at all, but instead to inject each module into the isolate.

Fixes #2553
Fixes #3559
Fixes #3751
Fixes #3825
Refs #3301

@kitsonk
Improvements to bundling.
a179053 
Moves to using a minimal System loader for bundles generated by Deno.
TypeScript in 3.8 will be able to output TLA for modules, and the loader
is written to take advantage of that as soon as we update Deno to TS
3.8.

System also allows us to support `import.meta` and provide more ESM
aligned assignment of exports, as well as there is better handling of
circular imports.

The loader is also very terse versus to try to save overhead.

Also, fixed an issue where abstract classes were not being rexported.

Fixes denoland#2553
Fixes denoland#3559
Fixes denoland#3751
Fixes denoland#3825
Refs denoland#3301
bartlomieju
bartlomieju reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, I defer to @ry to land

ry
ry reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@ry ry left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good.

It would be useful to have the test case from #3825 in the integration tests. (Note we already have //cli/tests/circular1.js, maybe that can be reused...)

@ry
Copy link
Member

ry commented Feb 11, 2020

Oops, I tried removing bundle_loader.js but it's used by mksnapshot_bundle... I wonder if mksnapshot_bundle couldn't also use this new system bundler?

@kitsonk
Copy link
Contributor Author

kitsonk commented Feb 11, 2020

@ry no... mentioned in the comments above:

It is worth noting that this cannot be used for the internal snapshot bundles, because it turns the whole instantiation process into an async process which executes out of turn, which causes some problems in setting up the runtime environment. The long term plan though for the snapshots is not to use a "bundle" at all, but instead to inject each module into the isolate.

In particular, the way we deal with Deno.core caused no end to problems when the bundle instantiated modules asynchronously. I know @bartlomieju tried to rework it to just inject each individual module, that would be better, but I know he ran into issues there too. I know it is a small bit of "duplication" but I think it is better to live with it for a period of time, too.

I will add a test case for #3825.

ry
ry approved these changes Feb 12, 2020
View reviewed changes
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 - thanks kitson!

@ry ry merged commit 6bd846a into denoland:master Feb 12, 2020
geoFlux added a commit to geoFlux/deno that referenced this pull request Feb 14, 2020
@geoFlux
Merge branch 'master' into ca_file
1637f5e 
* master:
  v0.33.0
  fix: appended CRLF to end of trailer headers (denoland#3989)
  Clean up fmt flags and path handling (denoland#3988)
  Improvements to bundling. (denoland#3965)
  fix: Correctly determine a --cached-only error (denoland#3979)
  chore: share HTTP server between tests (denoland#3966)
  dont use env vars in multiple installer tests (denoland#3967)
  feat(node): add EventEmitter.errorMonitor (denoland#3960)
  fix(file_server): don't crash on "%" pathname (denoland#3953)
  update references to testing/mod.ts in manual (denoland#3973)
@alexanderGalushka alexanderGalushka mentioned this pull request Feb 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju left review comments

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@kitsonk @ry @bartlomieju