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

add support for dynamic requests in require() and import() #7153

Merged
merged 18 commits into from
Feb 14, 2024

Conversation

sokra
Copy link
Member

@sokra sokra commented Jan 29, 2024

Description

Refactors PatternMatching to support a map of multiple matches by key. Based on the new RequestKey support for resolving.

Code Generation for require("./dir/"+ expr) will emit a map of __turbopack_lookup__({ "./a": () => ..., "./b": () => ... }, "./dir/" + expr).

The output format isn't really optimized yet, that's work for future PRs.

Testing Instructions

added a test case, enabled one previously skipped test case

Closes PACK-986

Copy link

vercel bot commented Jan 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-svelte-web 🔄 Building (Inspect) Visit Preview 💬 Add feedback Feb 14, 2024 3:29pm
rust-docs ❌ Failed (Inspect) Feb 14, 2024 3:29pm
9 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-gatsby-web ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-native-web ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-nonmonorepo ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-tailwind-web ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
examples-vite-web ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm
turbo-site ⬜️ Ignored (Inspect) Visit Preview Feb 14, 2024 3:29pm

Copy link
Contributor

github-actions bot commented Jan 29, 2024

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

✅ This change can build next-swc

Copy link
Contributor

github-actions bot commented Jan 29, 2024

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@sokra sokra marked this pull request as ready for review February 9, 2024 18:08
@sokra sokra requested a review from a team as a code owner February 9, 2024 18:08
crates/turbo-tasks/src/debug/mod.rs Show resolved Hide resolved
crates/turbopack-ecmascript/src/references/amd.rs Outdated Show resolved Hide resolved
crates/turbopack-ecmascript/src/references/amd.rs Outdated Show resolved Hide resolved
crates/turbopack-ecmascript/src/references/cjs.rs Outdated Show resolved Hide resolved
};
match args.into_iter().next() {
Some(ExprOrSpread { spread: None, expr: key_expr }) => {
*expr = pm.create_import(*key_expr, import_externals);
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if pm.create_import could return a CallExpr so we could still use the visit_mut_call_expr visitor

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically possible, but not sure if I want to put so much internal knowledge into the signature...

return Promise.resolve().then(() => {
throw e;
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return Promise.resolve().then(() => {
throw e;
});
return Promise.reject(e);

Copy link
Member Author

Choose a reason for hiding this comment

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

That's different. Promise.reject causes an unhandled rejection, but promise resolve throw allows to register a catch before the promise is rejected

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be the case?

You can try running this in node.js:

const process = require('node:process');

process.on('unhandledRejection', (reason, promise) => {
  console.log('Unhandled Rejection at:', promise, 'reason:', reason);
});

function willReject() {
  return Promise.reject(new Error("rejected with error"))
}

async function willCatch() {
  try {
    await willReject()
  } catch(e) {
    console.error("caught in try catch", e)
  }
}

willReject().catch((err) => console.error("caught in handler", err))

willCatch()

* flags:
* * 1: Error as rejected promise
*/
function moduleLookup(
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can actually just combine this with requireContext?
(for import we could add another property)

just need to copy the generic error from here and maybe expand commonJsRequireContext in place instead of having it as a separate thing

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we should combine them. In a followup PR?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can do that, will change all the snapshots again I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants