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

Fix loading circular dependencies #655

Merged
merged 2 commits into from
Sep 2, 2018
Merged

Fix loading circular dependencies #655

merged 2 commits into from
Sep 2, 2018

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Sep 1, 2018

Fixes #653

kitsonk
kitsonk commented Sep 1, 2018
View reviewed changes
js/compiler.ts
@@ -250,10 +251,6 @@ export class DenoCompiler implements ts.LanguageServiceHost {
}
const dependencyMetaData = this._getModuleMetaData(dep);
assert(dependencyMetaData != null, `Missing dependency "${dep}".`);
assert(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was, in the end, a bad assertion, because with circular dependencies, you will have some modules that are not yet run, but we will be returning the reference to their .exports object to a module.

ry
ry reviewed Sep 1, 2018
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.

Looks good. Thanks ! Is there any way this could be tested in compiler_test instead ? The check output tests are relatively expensive to run - so I would like to avoid adding to them if possible.

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 1, 2018

Yes, I can do that instead!

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 2, 2018

@ry good to go now.

ry
ry approved these changes Sep 2, 2018
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!

@ry ry merged commit 77faad8 into denoland:master Sep 2, 2018
@kitsonk kitsonk deleted the circular-deps branch August 2, 2022 04:42
hardfist pushed a commit to hardfist/deno that referenced this pull request Aug 7, 2024
@bartlomieju
feat: support for v8 code cache (denoland#655)
c7d9b2e 
This PR adds V8 code cache support for ES modules loaded through
`ModuleLoader`, and for scripts evaluated through `evalContext` (used by
`require` in Deno). Embedders have full control over whether code
caching is enabled, and how code cache is retrieved/stored.

---------

Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kitsonk @ry