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(core): splitting target path while coping content files #749

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

devpsk
Copy link

@devpsk devpsk commented Sep 1, 2024

Hi,

I noticed a bug when executing the command: npm run build aka (eventcatalog build). When I try to build the production version of the catalog, one of my application/microservice disappears. The application that disappears in the production environment (everything works on localhost) is the one named external-services-builder

During analysis, I found that the method getTargetPath is losing my name. The directory where Event Catalog places applications is services and my application / microservice also contains the word "services" (external-services-builder). So after calling the method "getTargetPath," an incorrect path is generated, which causes the application not to appear in the menu at all.

I analyzed the change history in the file scripts/catalog-to-astro-content-directory.js and noticed that it used to work previously. I restored that one code fragment.

Copy link

changeset-bot bot commented Sep 1, 2024

⚠️ No Changeset found

Latest commit: e566f06

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@@ -9,7 +9,7 @@ const scriptsDir = path.dirname(__filename);

const getTargetPath = (source, target, type, file) => {
const relativePath = path.relative(source, file);
const cleanedRelativePath = relativePath.split(type);
const cleanedRelativePath = relativePath.split(`${type}/`);
Copy link
Contributor

@carlosallexandre carlosallexandre Sep 1, 2024

Choose a reason for hiding this comment

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

The / was removed for compatibility with win32 plataform.
Maybe path.sep could works 🤷🏼‍♂️.

When I worked at a refactor proposal for the catalog-to-astro-content-directory.js I got the following for this snippet:

/**
 * Generates the path until the ASTRO_COLLECTION_KEY or the PROJECT_DIR root.
 * @param {string} filePath The path to the file.
 * @returns {string} The path until the COLLECTION_KEY or PROJECT_DIR root.
 */
const getRelativeTargetPath = (filePath) => {
  const filePathParsed = path.parse(filePath);
  const fileDir = filePathParsed.dir.split(path.sep).filter(Boolean);

  const relativePath = [];
  for (let i = fileDir.length - 1; i >= 0; i--) {
    relativePath.unshift(fileDir[i]);
    if (isCollectionKey(fileDir[i])) break;
  }

  return path.join(...relativePath, filePathParsed.base);
};

/**
 * Check if the key is an ASTRO COLLECTION KEY
 * @param {string} key
 * @returns {boolean}
 */
const isCollectionKey = (key) => {
  const COLLECTION_KEYS = ['commands', 'domains', 'events', 'pages', 'services', 'teams', 'users'];
  /** 
   *  Using includes won't works for your use case either
   *  The solution will be a comparison
   */
  return COLLECTION_KEYS.includes(key);
};

You can check the implementation here.
I hope this can help you find a suitable solution.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @carlosallexandre for review. I committed new changes, can you check?

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.

2 participants