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

chore(ssr-compiler): Align isValidIdentifier with babel-plugin-component impl #4964

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

jhefferman-sfdc
Copy link
Collaborator

Details

Fixes: #4826

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

W-17206167

@jhefferman-sfdc jhefferman-sfdc requested a review from a team as a code owner November 26, 2024 21:37
Comment on lines 58 to 59
"@types/estree": "^1.0.6",
"@types/babel-types": "^7.0.16"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"@types/estree": "^1.0.6",
"@types/babel-types": "^7.0.16"
"@types/estree": "^1.0.6",

We shouldn't need @types/babel-types, because that's for babel-types and we're using @babel/types. (Unless there's something weird going on with nested dependencies?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, thanks


export const isValidIdentifier = (str: string) =>
!reservedKeywords.has(str) && imperfectIdentifierMatcher.test(str);
export const isValidIdentifier = (str: string) => isValidES3Identifier(str);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're not adding any extra logic, we should probably remove this and just use the helper directly.

Copy link
Collaborator Author

@jhefferman-sfdc jhefferman-sfdc Nov 26, 2024

Choose a reason for hiding this comment

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

Do you mean the isIdentifierName helper? I checked the implementation and it seemed a little different in terms of reserved names, etc isIdentifierName vs isValidES3Identifier

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Will just means that we don't need to re-declare a const here – we can just use isValidES3Identifier directly.

BTW it'd be great to add a comment pointing to this code so it's clear why we are including a random Babel dep in our Meriyah code :)

const isInvalidMemberExpr = memberExprPaths.some(
(maybeIdentifier) =>
!(t.isValidES3Identifier(maybeIdentifier) && maybeIdentifier.length > 0)

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I was curious if the reservedKeywords actually matters here, and it looks like no. This is a valid object:

Click to see
const obj = {
  NaN: 'foo',
  arguments: 'foo',
  break: 'foo',
  case: 'foo',
  catch: 'foo',
  class: 'foo',
  const: 'foo',
  continue: 'foo',
  debugger: 'foo',
  default: 'foo',
  delete: 'foo',
  do: 'foo',
  else: 'foo',
  enum: 'foo',
  eval: 'foo',
  export: 'foo',
  extends: 'foo',
  false: 'foo',
  finally: 'foo',
  for: 'foo',
  function: 'foo',
  if: 'foo',
  implements: 'foo',
  import: 'foo',
  in: 'foo',
  instanceof: 'foo',
  interface: 'foo',
  let: 'foo',
  new: 'foo',
  null: 'foo',
  package: 'foo',
  private: 'foo',
  protected: 'foo',
  public: 'foo',
  return: 'foo',
  static: 'foo',
  super: 'foo',
  switch: 'foo',
  this: 'foo',
  throw: 'foo',
  true: 'foo',
  try: 'foo',
  typeof: 'foo',
  undefined: 'foo',
  var: 'foo',
  void: 'foo',
  while: 'foo',
  with: 'foo',
  yield: 'foo',
}

Copy link
Contributor

Choose a reason for hiding this comment

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

So we can remove reservedKeywords entirely since it's not used anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh sorry yes that sounds good, thank you both

@@ -51,7 +51,8 @@
"astring": "^1.9.0",
"estree-toolkit": "^1.7.8",
"immer": "^10.1.1",
"meriyah": "^5.0.0"
"meriyah": "^5.0.0",
"@babel/types": "7.26.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's alphabetize please :)

Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

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

Minor nitpicks, otherwise looks good!

@nolanlawson nolanlawson enabled auto-merge (squash) November 27, 2024 17:16
@nolanlawson nolanlawson merged commit f22b9b9 into master Nov 27, 2024
11 checks passed
@nolanlawson nolanlawson deleted the jhefferman/ssr/identifier-mechanism-consolidation branch November 27, 2024 17:30
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.

[SSR v2] Consolidate mechanisms that validate identifier/field names
3 participants