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

86byre0hx: Implemented sentry generator #23

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

Conversation

rbd2q
Copy link
Collaborator

@rbd2q rbd2q commented Aug 5, 2024

No description provided.

@rbd2q rbd2q self-assigned this Aug 5, 2024
@rbd2q rbd2q requested a review from dmitryusenko August 6, 2024 09:09
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.

@rbd2q Could you please address the following feedback?


Sentry.init({
// TODO replace with your DSN
dsn: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q Let's add one more question to the Sentry generator schema to prompt the user to enter the DSN, paste the provided DSN value here and remove the TODO comment above.


Sentry.init({
// TODO replace with your DSN
dsn: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q Let's add one more question to the Sentry generator schema to prompt the user to enter the DSN, paste the provided DSN value here and remove the TODO comment above.

import * as path from 'path';
import { SentryGeneratorSchema } from './schema';

const webDependencies = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q I'd suggest to rename this to nextAppDependencies.

Suggested change
const webDependencies = {
const nextAppDependencies = {

'@sentry/nextjs': '^8.21.0',
};

const mobileDependencies = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q I'd suggest to rename this to expoAppDependencies.

Suggested change
const mobileDependencies = {
const expoAppDependencies = {

) {
const projectRoot = `apps/${options.directory}`;

if (tree.exists(`${projectRoot}/next.config.js`)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q Let's create an utility function called isNextApp and use it here.

});

generateFiles(tree, path.join(__dirname, 'files'), projectRoot, options);
} else if (tree.exists(`${projectRoot}/metro.config.js`)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q Let's create an utility function called isExpoApp and use it here.

const nextConfigContent = tree
.read(`${projectRoot}/next.config.js`)
.toString()
.replace(
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q Unfortunately, the method of replacing anchor strings is unreliable and will eventually fail without warning. This can happen if someone changes the anchors, e.g. by adding or rearranging imports.

Can we instead work directly with the Abstract Syntax Tree (AST)? This would make the generator more stable and less reliant on specific anchors.

For more details and an example, see: https://nx.dev/extending-nx/recipes/modifying-files#ast-manipulation.


Sentry.init({
// TODO replace with your DSN
dsn: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

@rbd2q Let's add one more question to the Sentry generator schema to prompt the user to enter the DSN, paste the provided DSN value here and remove the TODO comment above.

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