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

#86c0b8vp3: Impement Sentry generator using ast #50

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

Conversation

RuslanAktaev
Copy link

No description provided.

@RuslanAktaev RuslanAktaev changed the title 86c0b8vp3 sentry generator with ast #86c0b8vp3: Impement Sentry generator using ast Oct 31, 2024
@RuslanAktaev
Copy link
Author

@dmitryusenko review please!

FYI @ipakhomov

Copy link
Contributor

@dmitryusenko dmitryusenko left a comment

Choose a reason for hiding this comment

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

@RuslanAktaev Looks good to me overall! I've provided the feedback above. Could you please address it?

@RuslanAktaev
Copy link
Author

@dmitryusenko fixed

@dmitryusenko
Copy link
Contributor

@RuslanAktaev Looks good to me, thanks!

@ipakhomov Could you please take a look?

@ipakhomov ipakhomov requested a review from kerne1s December 23, 2024 12:51
@ipakhomov
Copy link
Collaborator

@kerne1s could you please review?

tree: Tree,
options: SentryGeneratorSchema,
) {
const projectRoot = `apps/${options.directory}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't we use selectProject from utils? So we can remove x-prompt for directory.

Example: https://github.com/RonasIT/nx-generators/blob/main/plugin/src/generators/react-lib/generator.ts#L22

Comment on lines +15 to +17
const expoAppDependencies = {
'@sentry/react-native': '~6.1.0',
};
Copy link
Collaborator

@kerne1s kerne1s Dec 24, 2024

Choose a reason for hiding this comment

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

please move it to our dependencies file (Example: sentry > expo > ['@sentry/react-native': '~6.1.0'])

Comment on lines +9 to +11
const nextAppDependencies = {
'@sentry/nextjs': '^8.35.0',
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above

Comment on lines +3 to +5
export const isNextApp = (tree: Tree, projectRoot: string): boolean => {
return tree.exists(`${projectRoot}/next.config.js`);
};
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 combine these utils:

Suggested change
export const isNextApp = (tree: Tree, projectRoot: string): boolean => {
return tree.exists(`${projectRoot}/next.config.js`);
};
export type AppFramework = 'expo' | 'next';
export const isNextApp = (tree: Tree, projectRoot: string): boolean => {
return tree.exists(`${projectRoot}/next.config.js`);
};
export const isExpoApp = (tree: Tree, projectRoot: string): boolean => {
return tree.exists(`${projectRoot}/metro.config.js`);
};
export const getAppFrameworkName = (tree: Tree, projectRoot: string): AppFramework => {
return isNextApp(tree, projectRoot) ? 'next' : isExpoApp(tree, projectRoot) ? 'expo' : undefined;
};

@@ -0,0 +1,11 @@
import { Tree } from '@nx/devkit';

export const updateFile = (
Copy link
Collaborator

@kerne1s kerne1s Dec 24, 2024

Choose a reason for hiding this comment

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

we have a similar utility, can we modify it to use updateFile?

@kerne1s kerne1s assigned RuslanAktaev and unassigned ipakhomov Dec 24, 2024
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.

5 participants