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

Completed funcNameFromRelPathDefault #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LeadDreamer
Copy link

One issue, one style; both resolved by simplifying code:
Issue: the last replace, because it was a value not a regex, only replaced the first instance

Style: the string was already split; why .join(sep) only to .replace(`/${sep}/g, "-") later?

SO: split, then join("-"), then no need to replace.

Ran what tests I could (apparently don't have the full sample/test environment), but also tested "in the field" (emulator and deploy).

export function funcNameFromRelPathDefault(relPath: string): string {
  const relPathArray = relPath.split(sep); /* ? */
  const fileName = relPathArray.pop(); /* ? */
  const relDirPathFunctionNameChunk = relPathArray
    .map((pathFragment) => camelCase(pathFragment))
    .join("-");
  const fileNameFunctionNameChunk = camelCase(fileName!.split(".")[0]);
  const funcName = relDirPathFunctionNameChunk
    ? `${relDirPathFunctionNameChunk}-${fileNameFunctionNameChunk}`
    : fileNameFunctionNameChunk;
  return funcName;
}
```

@LeadDreamer
Copy link
Author

Runs successfully in emulation; have yet to deploy properly. Mis-read logs ealier.

@LeadDreamer
Copy link
Author

Deploy successful. Issue was unrelated (exact format of firebase-admin initializeApp - I use a centralized wrapper module for all firebase operations). Pull request valid.

@george43g
Copy link
Owner

This is the only PR which I havent been able to merge yet. Please check if this change still applies to the latest version.

@LeadDreamer
Copy link
Author

LeadDreamer commented May 9, 2022

So: I ended up converting to JS, since this was the only TS portion of my codebase. BUT I also both simplified it, and detected if the actual filename was "index.{wev}" - tossing that part if it was. I also did NOT reassemble with {sep} only to split it and rejoin it again later. If you never use "index.js" under a named directory, then my addition is not relevant. So I ended up:

function funcNameFromRelPathDefault(relPath) {
  if (!relPath) return ""; //short circuit

  let relPathArray = relPath.split(sep); /* ? */
  //check the last element for "index", and remove it if it is
  let filename = relPathArray.pop().toLowerCase().split(".").shift();
  if (filename === "index") {
    filename = relPathArray.pop().toLowerCase().split(".").shift();
  }
  relPathArray.push(filename);
  //camelcase all the elements, and join with "-"
  const funcName = relPathArray.map((element) => camelCase(element)).join("-");

  return funcName;
}

@hursey013
Copy link

This still appears to be an issue for me using the latest release. Here's what I need to do in order to get it working:

const { exportFunctions } = require('better-firebase-functions')
const camelCase = require('camelcase')
const { sep } = require('path')

exportFunctions({
  __filename,
  exports,
  funcNameFromRelPath: relPath => {
    const relPathArray = relPath.split(sep)
    const fileName = relPathArray.pop()
    const relDirPathFunctionNameChunk = relPathArray
      .map(pathFragment => camelCase(pathFragment))
      .join(sep)
    const fileNameFunctionNameChunk = camelCase((fileName || '').split('.')[0])
    const funcName = relDirPathFunctionNameChunk
      ? `${relDirPathFunctionNameChunk}${sep}${fileNameFunctionNameChunk}`
      : fileNameFunctionNameChunk

    return funcName.split(sep).join('-')
  }
})

Without that, I receive the following error:

... function name(s) can only contain letters, numbers, hyphens, and not exceed 62 characters in length

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