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 RegExp in expandReferences #875

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Fix RegExp in expandReferences #875

merged 3 commits into from
Jan 9, 2025

Conversation

josephjclark
Copy link
Collaborator

This PR fixes an issue where passing a RegExp to expandReferences causes it to be serialised into a plan object

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

@josephjclark
Copy link
Collaborator Author

Hey @decarteret - this should fix the regex thing on your branch. Can you test here and merge down? Thanks!

@decarteret
Copy link
Collaborator

I tested this fix and it may not be all that's needed or may require further discussion.

In my test, in my job code, I have:

const expression = /test/;

fn(state => {
  console.log("outsideExpression value:", expression);
  console.log("outsideExpression instanceof RegExp:", expression instanceof RegExp);
  console.log("outsideExpression type toString:", Object.prototype.toString.call(expression));
});

getContentsFromMessage("userId", "query", expression); // here, expression is parameter desiredContents

Inside getContentsFromMessage I added this test code:

const insideExpression = /test/;
console.log("insideExpression value:", insideExpression);
console.log("insideExpression instanceof RegExp:", insideExpression instanceof RegExp);
console.log("insideExpression type toString:", Object.prototype.toString.call(insideExpression));

const passedExpression = desiredContents; // from function parameter
console.log("passedExpression value:", passedExpression);
console.log("passedExpression instanceof RegExp:", passedExpression instanceof RegExp);
console.log("passedExpression type toString:", Object.prototype.toString.call(passedExpression));

const resolvedPassedExpression = resolvedDesiredContents; // resolved from expandReferences
console.log("resolvedPassedExpression value:", resolvedPassedExpression);
console.log("resolvedPassedExpression instanceof RegExp:", resolvedPassedExpression instanceof RegExp);
console.log("resolvedPassedExpression type toString:", Object.prototype.toString.call(resolvedPassedExpression));

The output is:

outsideExpression value: {}
outsideExpression instanceof RegExp: true
outsideExpression type toString: [object RegExp]

insideExpression value: /test/
insideExpression instanceof RegExp: true
insideExpression type toString: [object RegExp]

passedExpression value: /test/
passedExpression instanceof RegExp: false
passedExpression type toString: [object RegExp]

resolvedPassedExpression value: {}
resolvedPassedExpression instanceof RegExp: false
resolvedPassedExpression type toString: [object Object]

Key takeaways for expected values:

  • outsideExpression value should be /test/, not {}, but somehow the other values are still correct
  • insideExpression is correct in all cases
  • passedExpression instanceof RegExp SHOULD be true, but is somehow false, even though the type toString is correct
  • resolvedPassedExpression therefore is all incorrect because of the previous failures

@josephjclark
Copy link
Collaborator Author

Thank you for the detailed report @decarteret ! Let me take a look and come back to you

@josephjclark
Copy link
Collaborator Author

Ok @decarteret that kinda blew my stack.

There's some kind of weird identity thing where inside the vm, typeof RegExp consistently fails. Grokking that is beyond my ken tbh BUT I've got a good workaround which seems to work.

Could you retest for me?

BTW, in your test code, don't worry that console.log(/./) outputs {}. That's to do with how console.log messages are serialized out to the console. Call it a bug in the logger if you like. It's not really related to this issue.

@decarteret
Copy link
Collaborator

Success! With your new changes I reran my tests and it's working perfectly now! Thank you so much for spending time on this!

@decarteret decarteret closed this Jan 9, 2025
@decarteret decarteret reopened this Jan 9, 2025
@decarteret decarteret merged commit aa654ab into nhgh/gmail Jan 9, 2025
3 checks passed
@decarteret decarteret deleted the expand-regex branch January 9, 2025 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants