Skip to content

Conversation

@CaptainNic
Copy link
Contributor

Summary

The jest HasteImpl's pluginNameReducers regex was not properly escaping a backslash, resulting in unintended behavior.

The intent of the code is to strip the .${name} from the end of a filepath, where name is a platform name. The correct regex for that would be ^(.*)\.(myPlat)$, but because the regex is being constructed from a string, the \. is being interpreted as an escaped period, resulting in the regex ^(.*).(myPlat)$. To correct this, the backslash needs to be escaped so it makes it into the regex.

Changelog

[General] [Fixed] - Fix HasteImpl platform name regex

Test Plan

This is a stripped down version of jest/hasteImpl.js for testing. Can also be found at https://codepen.io/anon/pen/wZNovr?editors=0011

const haste = {
  platforms: ['ios', 'android', 'dummy']
};

const oldPluginNameReducers = haste.platforms.map(
  name => [new RegExp(`^(.*)\.(${name})$`), '$1'],
);

const newPluginNameReducers = haste.platforms.map(
  name => [new RegExp(`^(.*)\\.(${name})$`), '$1'],
);

function getHasteName(filePath, sourceCode) {
  console.log(`getHasteName('${filePath}')`);
  
  const oldHasteName = oldPluginNameReducers.reduce(
    (name, [pattern, replacement]) => name.replace(pattern, replacement),
    filePath,
  );
  console.log(`[OLD] ${oldHasteName}`);

  const newHasteName = newPluginNameReducers.reduce(
    (name, [pattern, replacement]) => name.replace(pattern, replacement),
    filePath,
  );
  console.log(`[NEW] ${newHasteName}`);
};

getHasteName('myComponent.ios');
getHasteName('myComponentios');

getHasteName('myComponent.dummy');
getHasteName('myComponent.ddummy');

Output:

getHasteName('myComponent.ios')
[OLD] myComponent
[NEW] myComponent
getHasteName('myComponentios')
[OLD] myComponen
[NEW] myComponentios
getHasteName('myComponent.dummy')
[OLD] myComponent
[NEW] myComponent
getHasteName('myComponent.ddummy')
[OLD] myComponent.
[NEW] myComponent.ddummy

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot facebook-github-bot added the p: Sony Partner: Sony label Apr 26, 2019
@cpojer
Copy link
Contributor

cpojer commented Apr 26, 2019

Thank you @CaptainNic. Do you mind signing the CLA so I can merge this PR?

@CaptainNic
Copy link
Contributor Author

@cpojer I'm working on figuring out the CLA issue, I was under the assumption that I was already covered under my company's CLA.

@cpojer
Copy link
Contributor

cpojer commented Apr 26, 2019

You may need to talk to somebody at your company to get yourself added. I'm assuming @matthargett will be able to help you out :)

@react-native-bot react-native-bot added No CLA Authors need to sign the CLA before a PR can be reviewed. Bug labels Apr 27, 2019
@matthargett
Copy link
Contributor

@cpojer @hramos Joel at FB emailed back and said the CLA list was updated. Please let us know if there's anything else we need to do to get this merged. Thanks!

pluginNameReducers was not properly escaping a backslash, resulting in unintended regex behavior.
@CaptainNic CaptainNic force-pushed the bugfix/hasteImpl-regex branch from 16e5c34 to 3c257fa Compare May 6, 2019 15:20
@hramos
Copy link
Contributor

hramos commented May 6, 2019

I tried to merge this, but the import tool is unable to find a valid CLA on file. I just looked around our CLA tool and could not find @CaptainNic listed in the CLA. Let me follow up internally and see what's up.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 6, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@react-native-bot react-native-bot removed the No CLA Authors need to sign the CLA before a PR can be reviewed. label May 6, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @CaptainNic in 28e0de0.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 6, 2019
grabbou pushed a commit that referenced this pull request May 7, 2019
Summary:
The jest HasteImpl's `pluginNameReducers` regex was not properly escaping a backslash, resulting in unintended behavior.

The intent of the code is to strip the `.${name}` from the end of a filepath, where `name` is a platform name. The correct regex for that would be `^(.*)\.(myPlat)$`, but because the regex is being constructed from a string, the `\.` is being interpreted as an escaped period, resulting in the regex  `^(.*).(myPlat)$`. To correct this, the backslash needs to be escaped so it makes it into the regex.

[General] [Fixed] - Fix HasteImpl platform name regex
Pull Request resolved: #24628

Differential Revision: D15224468

Pulled By: hramos

fbshipit-source-id: 6eb507aa5410bdd7c247e6d301052d41995a2f11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Sony Partner: Sony

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants