Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

[expo-cli] turtle-v2 credentials change hasLocal,hasRemote bahviour #2452

Merged
merged 1 commit into from
Aug 12, 2020

Conversation

wkozyra95
Copy link
Contributor

@wkozyra95 wkozyra95 commented Aug 12, 2020

why

It's not clear what credentials are used during build, misconfiguration of credentials.json might cause expo-cli to use remote ones without any info for the user

@wkozyra95 wkozyra95 requested a review from dsokal August 12, 2020 10:32
Copy link
Contributor

@dsokal dsokal left a comment

Choose a reason for hiding this comment

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

Please update the changelog.

credentialsJSONRaw = JSON.parse(credentialsJSONContents);
} catch (err) {
throw new Error(
`credentials.json must exist in the project root directory and consist a valid JSON`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`credentials.json must exist in the project root directory and consist a valid JSON`
`credentials.json must exist in the project root directory and contain a valid JSON`

`credentials.json must exist in the project root directory and consist a valid JSON`
);
}
return credentialsJSONRaw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return credentialsJSONRaw;

let credentialsJSONRaw;
try {
const credentialsJSONContents = await fs.readFile(credentialsJsonFilePath, 'utf8');
credentialsJSONRaw = JSON.parse(credentialsJSONContents);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
credentialsJSONRaw = JSON.parse(credentialsJSONContents);
return JSON.parse(credentialsJSONContents);

@@ -116,7 +107,21 @@ async function readAsync(projectDir: string): Promise<CredentialsJson> {
return credentialsJson;
}

async function readRawAsync(projectDir: string): Promise<any> {
const credentialsJsonFilePath = path.join(projectDir, 'credentials.json');
let credentialsJSONRaw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let credentialsJSONRaw;

Comment on lines 15 to 24
//const originalWarn = console.warn;
//const originalLog = console.log;
//beforeAll(() => {
// console.warn = jest.fn();
// console.log = jest.fn();
//});
//afterAll(() => {
// console.warn = originalWarn;
// console.log = originalLog;
//});
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot to uncomment

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/change-detecting-local-credentials branch 2 times, most recently from 3f93d29 to 5617672 Compare August 12, 2020 10:59
@wkozyra95 wkozyra95 requested a review from dsokal August 12, 2020 11:07
@wkozyra95 wkozyra95 merged commit d75089b into master Aug 12, 2020
@wkozyra95 wkozyra95 deleted the @wkozyra95/change-detecting-local-credentials branch August 12, 2020 11:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants