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: handle commonjs/es import styles for @sentry/cli #302

Merged
merged 2 commits into from Apr 5, 2021

Conversation

snake575
Copy link
Contributor

@snake575 snake575 commented Apr 4, 2021

Fixes #301

@codecov
Copy link

codecov bot commented Apr 4, 2021

Codecov Report

Merging #302 (d64249b) into master (eed66db) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #302   +/-   ##
=======================================
  Coverage   55.55%   55.55%           
=======================================
  Files           1        1           
  Lines          27       27           
  Branches        8        8           
=======================================
  Hits           15       15           
  Misses          9        9           
  Partials        3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eed66db...d64249b. Read the comment docs.

lib/core/hooks.js Outdated Show resolved Hide resolved
Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>
@snake575
Copy link
Contributor Author

snake575 commented Apr 4, 2021

@rchl I noticed that @sentry/webpack-plugin already gets the release version with proposeVersion by default:

https://github.com/getsentry/sentry-webpack-plugin/blob/fbd610c22abf94b41f3056d7f69fb32a96e95314/src/index.js#L86

Maybe it's not necessary to implement the same feature on this module?

@rchl
Copy link
Member

rchl commented Apr 4, 2021

@rchl I noticed that @sentry/webpack-plugin already gets the release version with proposeVersion by default:

getsentry/sentry-webpack-plugin@fbd610c/src/index.js#L86

Maybe it's not necessary to implement the same feature on this module?

I think the release needs to be set in the config that is generated at build-time even if publishRelease is enabled as webpack-plugin won't update the module's config.

Feel free to correct me if I'm wrong as I don't fully remember how everything works now.

@snake575
Copy link
Contributor Author

snake575 commented Apr 4, 2021

I think the release needs to be set in the config that is generated at build-time even if publishRelease is enabled as webpack-plugin won't update the module's config.

Feel free to correct me if I'm wrong as I don't fully remember how everything works now.

I made new branch deleting the implementation to test this (it works). Maybe was implemented to fix some edge case?

master...snake575:sentry-webpack-default-release

The uploaded release matches this branch last commit ID

https://github.com/snake575/nuxt-sentry/tree/sentry-webpack-default-release

https://github.com/snake575/nuxt-sentry/commit/2ae0ea493bf09fc219661f77c535b88eca2b5e1c

image

@rchl
Copy link
Member

rchl commented Apr 4, 2021

But are issues reported with the correct version still?

Besides, I think that even when you have not enabled publishRelease, you still want the correct version to be reported in the issues.

@snake575
Copy link
Contributor Author

snake575 commented Apr 5, 2021

You're right, on the branch eliminating the implementation, issues get reported on sentry but not on any release.

@rchl rchl changed the title fix: remove default from @sentry/cli import fix: handle commonjs/es import styles for @sentry/cli Apr 5, 2021
@rchl rchl merged commit d6a818d into nuxt-community:master Apr 5, 2021
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.

Automatic config.release from @sentry/cli fails because of undefined import
2 participants