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(cli): migrate run and cache to new infrastructure #7996

Merged
merged 14 commits into from
Oct 23, 2020

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Oct 16, 2020

Ref: #7225

Still a work in progress, the biggest challenge is with dynamic imports. I need to do some thinking about how best to integrate these in a way that is clean in the new infrastructure.

There is still a bit of cleanup and some more unit tests to write, but thought it was good enough. It is a ~420 LOC reduction at the moment, but there is certainly more that will be done when bundling can the runtime APIs move over.

@bartlomieju
Copy link
Member

Still a work in progress, the biggest challenge is with dynamic imports. I need to do some thinking about how best to integrate these in a way that is clean in the new infrastructure.

Can you elaborate what's the hurdle with dynamic imports?

@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 19, 2020

Can you elaborate what's the hurdle with dynamic imports?

Because the module graph is the centre of the universe, it means that it tries to go get dynamic imports eagerly, when swc sees them, also, instead of waiting until they are demanded by the runtime. tsc also tries to use them in type checking, so we want to be able to eagerly fetch them. Which means we encounter all the errors there, during the building of the graph, instead when v8 attempts to loads them. This lead to errors not being catchable. I have fixed it now in my branch, basically making dynamic import resolutions in the graph non-errors, but as I said in our chat, I broke a few other things, but I think I know what happened there and am fixing that now.

cli/module_graph2.rs Outdated Show resolved Hide resolved
@kitsonk kitsonk marked this pull request as ready for review October 21, 2020 05:13
@kitsonk kitsonk changed the title [WIP] refactor(cli): migrate run and cache to new infrastructure refactor(cli): migrate run and cache to new infrastructure Oct 21, 2020
@kitsonk kitsonk added the breaking change a change or feature that breaks existing semantics label Oct 21, 2020
@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 21, 2020

I am pretty happy with it now.

It does include the breaking change that isolatedModules are on by default, which has only been the case under --unstable since 1.4.

I am going back through issues that we said we wanted to address with this re-write and writing some test for them. At least one of them is not fixed and I am looking into what I can do to fix it at the moment.

@kitsonk kitsonk removed the breaking change a change or feature that breaks existing semantics label Oct 21, 2020
cli/module_graph2.rs Show resolved Hide resolved
cli/module_graph2.rs Show resolved Hide resolved
cli/module_graph2.rs Show resolved Hide resolved
cli/module_loader.rs Show resolved Hide resolved
cli/program_state.rs Outdated Show resolved Hide resolved
cli/program_state.rs Show resolved Hide resolved
cli/specifier_handler.rs Outdated Show resolved Hide resolved
cli/tests/disallow_http_from_https_ts.out Show resolved Hide resolved
cli/tests/error_local_static_import_from_remote.js.out Outdated Show resolved Hide resolved
cli/source_maps.rs Show resolved Hide resolved
cli/specifier_handler.rs Outdated Show resolved Hide resolved
@kitsonk
Copy link
Contributor Author

kitsonk commented Oct 22, 2020

@bartlomieju I think I have addressed all your feedback... PTAL

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, let's land it 🚀

cli/specifier_handler.rs Show resolved Hide resolved
@kitsonk kitsonk requested a review from ry October 22, 2020 20:05
@kitsonk kitsonk merged commit 7e2c7fb into denoland:master Oct 23, 2020
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.

4 participants