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 require statements with plain template literals #7369

Merged
merged 5 commits into from Nov 28, 2021

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Nov 26, 2021

↪️ Pull Request

Converts plain template literals into a string so dependency collector does find the dependency.

Fixes #7368
Fixes #5492

💻 Examples

export const startLogger = () => {
  require("./test");
  if (process.env.GATSBY_LOGGER.includes(`json`)) {
    // TODO move to dynamic imports
    require(`./logger/test.js`).initializeJSONLogger();
  } else if (process.env.GATSBY_LOGGER.includes(`yurnalist`)) {
    // TODO move to dynamic imports
    require(`./logger/test.js`).initializeYurnalistLogger();
  } else {
    // TODO move to dynamic imports
    require(`./logger/test.js`).initializeINKLogger();
  }
}

🚨 Test instructions

✔️ 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

@height
Copy link

height bot commented Nov 26, 2021

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@wardpeet wardpeet changed the title Fix require statements with string literal Fix require statements with plain template literals Nov 26, 2021
Comment on lines +542 to +554
let mut arg = arg.clone();

// convert require(`./name`) to require("./name")
if let ast::Expr::Tpl(_tpl) = &*arg.expr {
if _tpl.quasis.len() == 1 && _tpl.exprs.is_empty() {
let tpl_str = &_tpl.quasis[0].raw;
arg.expr = Box::new(ast::Expr::Lit(ast::Lit::Str(ast::Str {
value: tpl_str.clone().value,
..tpl_str.clone()
})));
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really familiar with Rust so if this code is not optimized or can be rewritten to something better, I'll take it :)

@mischnic
Copy link
Member

Can you add 2 new tests: one for the test you already changed (and revert that existing one), and one test that does something like this (which should still be ignored)

let x = 2;
require(`foo${x}`)

@wardpeet
Copy link
Contributor Author

Updated, found another piece that needed to be updated.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thanks! We could probably make a util function for this but I can do that separately.

@devongovett devongovett merged commit b08ef9c into parcel-bundler:v2 Nov 28, 2021
@devongovett
Copy link
Member

Here's that util: #7376

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