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: resolveProjectBasePath could not find source root when directory… #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JelleBruisten
Copy link
Contributor

… name and project name are different

prior to this change we would look for **/<projectName> however this does not work when we have a folder structure like: /libs/ui/my-button where the project name would be ui-my-button causing the keys not to be detected this change would look for all project.json files and look for the correct project.json by comparing the name property within

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Currently when having a folder structure where the project name does not match the folder name transloco-key-manager is unable to find the source root.

for example if we would have
libs/booking/ui/button when looking for a project with the name booking-ui-button transloco won't find the source root of this project causing the error:
image

Issue Number: N/A

What is the new behavior?

with the changes in this PR we look for all project.json and compare the name property to find out if it is the correct config file.

this enables us to have folder structures like
/booking/ui/button
/booking/ui/card
/invoicing/ui/button

And before when running npx transloco-keys-manager extract --project=button it would match one of these libraries but never the other. This change would also make this possible where everything is correctly matched based on the project name that is specified in project.json ( project name should still be unique in each workspace else nx will not work correctly either ).

Does this PR introduce a breaking change?

[ ] Yes
[X ] No

## Other information

… name and project name are different

prior to this change we would look for `**/<projectName>` however this does not work when we have a folder structure like:
`/libs/ui/my-button` where the project name would be `ui-my-button` causing the keys not to be detected
this change would look for all project.json files and look for the correct project.json by comparing the name property within
@stackblitz
Copy link

stackblitz bot commented Apr 23, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@evanfuture
Copy link
Contributor

Hi, any reason why this hasn't been merged? I was wondering why some keys on my project weren't being extracted. Turns out, a lot weren't because of this. Applied it locally and works fine...

@shaharkazaz
Copy link
Collaborator

I'll try to review this week 👍

Comment on lines +47 to 61
let projectConfig;
if (projectName) {
projectPath = normalizedGlob(`**/${projectName}`)[0];
// look for all project.json files and find the project.json that contains
// { name: "<projectName>" ... }
const projectConfigs = normalizedGlob(`**/${projectConfigFile}`);
for(const p of projectConfigs) {
const config = searchConfig(projectConfigFile, p);
if(config.name === projectName) {
projectConfig = config;
break;
}
}
} else {
projectConfig = searchConfig(projectConfigFile, projectPath);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move this into resolveProjectConfig and merge 👍

Copy link
Collaborator

@shaharkazaz shaharkazaz left a comment

Choose a reason for hiding this comment

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

Can you please add tests for the case you are solving? there is a test file for resolving configurations.
Also, please merge master so the CI can run 🙂

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