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

Fix transformation of import/requires wrapped into Promise.resolve(). #8167

Merged
merged 2 commits into from
Jun 13, 2022

Conversation

SmartArray
Copy link
Contributor

@SmartArray SmartArray commented May 29, 2022

↪️ Pull Request

Previously we noticed that parcel incorrectly transforms Promise.resolve(import('foo')).then(...) as described in #8116.

The transformer that rewrites the nodes of the code Promise.resolve().then(() => return require('foo')) erroneously restructures the above code too.
However, there is no need to transform this require/import statement, when wrapped into Promise.resolve() because it is used to promisify the import in the first place.

This PR adds a unit test to illustrate the issue, and a condition that prevents the incorrect processing of the statement.

💻 Examples

// main.js
module.exports = Promise.resolve(require('./async')).then(x => x + 1335);
// async.js
module.exports = 2;

Expected behaviour

const n = await import('./main');
n === 1337

Current behaviour

The current code causes the runtime to crash because it injects a variable called res incorrectly, and thus destroys the code.

🚨 Test instructions

yarn workspace @parcel/integration-tests test -g Promise.resolve

Or use the minimal reproduction code here

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

There is no need to transform imports that are wrapped into Promise.resolve() because in that case it does not matter if the import is a Promise or not.
@devongovett devongovett merged commit 3f02a9a into parcel-bundler:v2 Jun 13, 2022
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.

3 participants