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

minor: exclude esm-env from deps #153

Merged
merged 4 commits into from
Nov 16, 2023
Merged

minor: exclude esm-env from deps #153

merged 4 commits into from
Nov 16, 2023

Conversation

jill64
Copy link
Owner

@jill64 jill64 commented Nov 16, 2023

close #147

Summary by CodeRabbit

  • Documentation

    • Updated the README to enhance clarity and provide quick insights with new badges for npm version, license, monthly downloads, and minified size.
  • Refactor

    • Streamlined the handling of external dependencies in build configurations using a wildcard pattern.
    • Updated import statements for environment variables to align with the new reference paths in both client and server initialization code.
  • Style

    • Renamed the "Install" section to "Installation" in the README for better readability.

@wraith-ci wraith-ci bot enabled auto-merge November 16, 2023 10:04
Copy link

coderabbitai bot commented Nov 16, 2023

Walkthrough

The updates across the sentry-sveltekit-cloudflare package reflect a shift in configuration and documentation. The README.md is enhanced with badges for better package insights at a glance. The build script and initialization files for both client and server are updated to use a new import source for the development environment variable, streamlining the development mode detection process.

Changes

File(s) Change Summary
README.md Updated "Install" to "Installation" and added badges for npm version, license, monthly downloads, and minified size.
scripts/build.ts Modified external configuration to use a wildcard pattern for $app/ modules.
src/client/init.ts, src/server/init.ts Changed import source from esm-env to $app/environment and updated the development mode check.

🐇 In the code's gentle flow, 🍂
Changes come and badges glow.
As leaves descend in twirling dance,
Our updates join the autumn's prance. 🌟


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link

cloudflare-workers-and-pages bot commented Nov 16, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 494e9df
Status: ✅  Deploy successful!
Preview URL: https://8fe4f310.sentry-sveltekit-cloudflare.pages.dev
Branch Preview URL: https://issue-147.sentry-sveltekit-cloudflare.pages.dev

View logs

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a59fdc7 and 6e62305.
Files ignored due to filter (2)
  • package.json
  • pnpm-lock.yaml
Files selected for processing (4)
  • README.md (1 hunks)
  • scripts/build.ts (1 hunks)
  • src/client/init.ts (2 hunks)
  • src/server/init.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • README.md
Additional comments: 5
src/server/init.ts (2)
  • 1-4: The import statement has been updated to reflect the new source of the development environment variable. This change should be verified to ensure that $app/environment is the correct and intended source for this variable in the context of the SvelteKit framework.

  • 33-39: The conditional logic has been updated to use the new dev variable. This change is consistent with the updated import statement. Ensure that the behavior of dev from $app/environment matches the expected behavior of the previous DEV variable to prevent any unintended side effects in development mode.

scripts/build.ts (1)
  • 6-10: The change to the external field in the build configuration is significant. By excluding all modules under the $app namespace, it ensures that these modules are not bundled into the final build. This is a common practice when working with SvelteKit, as $app modules are provided by the SvelteKit runtime and should not be bundled with the application code. This change should be verified to ensure that it does not unintentionally exclude modules that should be part of the bundle, which could break the application if they are not available in the runtime environment.
src/client/init.ts (2)
  • 2-7: The import changes from esm-env to $app/environment are consistent with the summary provided. This change should be verified to ensure that $app/environment is the correct source for the dev variable and that it is compatible with the rest of the codebase.

  • 30-35: The conditional logic has been updated to use the new dev variable. It's important to ensure that the behavior of the dev variable from $app/environment matches the expected behavior previously provided by DEV from esm-env. If dev behaves differently, additional changes might be required to maintain the same functionality.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a59fdc7 and 5376b38.
Files ignored due to filter (2)
  • package.json
  • pnpm-lock.yaml
Files selected for processing (4)
  • README.md (1 hunks)
  • scripts/build.ts (1 hunks)
  • src/client/init.ts (2 hunks)
  • src/server/init.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • README.md
Additional comments: 4
src/client/init.ts (2)
  • 5-5: The import statement has been correctly updated to use $app/environment for the dev variable, which aligns with SvelteKit's environment handling conventions.

  • 33-35: The conditional check for development mode has been updated to use the new dev variable. This change is consistent with the updated import and maintains the existing logic, which only initializes Sentry if enableInDevMode is true or if the environment is not development (!dev).

src/server/init.ts (2)
  • 1-4: The change from esm-env to $app/environment for importing the dev variable aligns with SvelteKit's convention for handling environment variables. This is a good practice as it ensures consistency with the framework's standards.

  • 33-39: The conditional logic has been updated to use the new dev variable. It's important to ensure that the rest of the codebase that relies on the DEV constant is also updated to use the new dev variable from $app/environment if applicable. Additionally, the logic here assumes that options can be undefined, which is safely handled with the nullish coalescing operator. This is a good use of modern JavaScript features for better code safety and readability.

Comment on lines 6 to 10
outfile: 'dist/index.js',
minify: true,
format: 'esm',
external: ['esm-env', '$app/stores']
external: ['$app/*']
})
Copy link

Choose a reason for hiding this comment

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

The change to use a wildcard pattern for external modules is a significant one. It's important to ensure that this does not unintentionally exclude any modules that should be bundled with the package. This change could improve build performance by reducing the bundle size and avoiding bundling of unnecessary modules. However, it's crucial to verify that all necessary modules are still correctly resolved and included in the final build where needed.

-  external: ['module1', 'module2', 'module3']
+  external: ['$app/*']

Commitable suggestion

[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
outfile: 'dist/index.js',
minify: true,
format: 'esm',
external: ['esm-env', '$app/stores']
external: ['$app/*']
})
outfile: 'dist/index.js',
minify: true,
format: 'esm',
external: ['$app/*']
})

@wraith-ci wraith-ci bot merged commit 6671352 into main Nov 16, 2023
4 checks passed
@wraith-ci wraith-ci bot deleted the issue-147 branch November 16, 2023 11:10
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 13ab979 and 494e9df.
Files ignored due to filter (2)
  • package.json
  • pnpm-lock.yaml
Files selected for processing (4)
  • README.md (1 hunks)
  • scripts/build.ts (1 hunks)
  • src/client/init.ts (2 hunks)
  • src/server/init.ts (2 hunks)
Files skipped from review due to trivial changes (2)
  • README.md
  • scripts/build.ts
Additional comments: 4
src/client/init.ts (2)
  • 5-5: The import statement has been updated to use $app/environment instead of esm-env. This change aligns with SvelteKit's conventions and should work as long as the $app/environment module provides the dev variable as expected.

  • 30-35: The conditional logic here checks if the environment is development and if enableInDevMode is not true, it returns the default error handler without initializing Sentry. This is a sensible default to prevent Sentry from being initialized in development environments unless explicitly requested.

src/server/init.ts (2)
  • 1-4: The import statement has been correctly updated to use SvelteKit's environment modules. This aligns with the framework's conventions and should work as expected, provided that $app/environment is correctly configured in the SvelteKit project.

  • 33-39: The conditional logic has been updated to use the new dev variable from $app/environment. This change is consistent with the updated import and should maintain the existing functionality, assuming that dev correctly reflects whether the code is running in development mode. It's important to ensure that the behavior of dev matches the previous DEV variable to avoid unexpected behavior.

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.

omit esm-env from deps
1 participant