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: better handling of functions options in config #267

Merged
merged 3 commits into from Feb 7, 2021

Conversation

charlie0228
Copy link
Contributor

Fixes #266
Check config when the option is a function then using serializeFunction(option) instead of using serialize(option)

@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #267 (aaaa8b3) into master (93ae748) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #267   +/-   ##
=======================================
  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 93ae748...aaaa8b3. Read the comment docs.

@guAnsunyata
Copy link

It works for me!

@rchl
Copy link
Member

rchl commented Jan 21, 2021

This doesn't seem to help.

With a configuration like:

  sentry: {
    dsn: 'https://fe8b7df6ea7042f69d7a97c66c2934f7@sentry.io.nuxt/1429779',
    config: {
      beforeSend (event, hint) {
        const ignoreErrorInBeforeSend = [
          {
            check: (event, hint) => {
              // Ignore fbevent.js postMessage error in iOS device
              if (hint.originalException.stack) {
                // eslint-disable-next-line dot-notation
                const errorStack = JSON.stringify(hint.originalException.stack)
                return /postMessage/.test(errorStack)
              }

              return false
            }
          }
        ]

        const shouldIgnoreError = ignoreErrorInBeforeSend.some(rule =>
          rule.check(event, hint)
        )
        return shouldIgnoreError ? null : event
      }
    },
  }

it produces:

  const config = {
    dsn:"https:\u002F\u002Ffe8b7df6ea7042f69d7a97c66c2934f7@sentry.io.nuxt\u002F1429779",
    environment:"development",
    beforeSend:beforeSend (event, hint) {
        const ignoreErrorInBeforeSend = [
          {
            check: (event, hint) => {
              // Ignore fbevent.js postMessage error in iOS device
              if (hint.originalException.stack) {
                // eslint-disable-next-line dot-notation
                const errorStack = JSON.stringify(hint.originalException.stack)
                return /postMessage/.test(errorStack)
              }

              return false
            }
          }
        ]

        const shouldIgnoreError = ignoreErrorInBeforeSend.some(rule =>
          rule.check(event, hint)
        )
        return shouldIgnoreError ? null : event
      },
    release:"a21baf77b07df7892b8dcac059ef4e39c53d5eb7"
  }

which is still invalid due to beforeSend:beforeSend (event, hint) { part. Should be beforeSend:function (event, hint) {

@charlie0228
Copy link
Contributor Author

Thanks for your review!
Let me check my PR again.

@charlie0228
Copy link
Contributor Author

This doesn't seem to help.

With a configuration like:

  sentry: {
    dsn: 'https://fe8b7df6ea7042f69d7a97c66c2934f7@sentry.io.nuxt/1429779',
    config: {
      beforeSend (event, hint) {
        const ignoreErrorInBeforeSend = [
          {
            check: (event, hint) => {
              // Ignore fbevent.js postMessage error in iOS device
              if (hint.originalException.stack) {
                // eslint-disable-next-line dot-notation
                const errorStack = JSON.stringify(hint.originalException.stack)
                return /postMessage/.test(errorStack)
              }

              return false
            }
          }
        ]

        const shouldIgnoreError = ignoreErrorInBeforeSend.some(rule =>
          rule.check(event, hint)
        )
        return shouldIgnoreError ? null : event
      }
    },
  }

it produces:

  const config = {
    dsn:"https:\u002F\u002Ffe8b7df6ea7042f69d7a97c66c2934f7@sentry.io.nuxt\u002F1429779",
    environment:"development",
    beforeSend:beforeSend (event, hint) {
        const ignoreErrorInBeforeSend = [
          {
            check: (event, hint) => {
              // Ignore fbevent.js postMessage error in iOS device
              if (hint.originalException.stack) {
                // eslint-disable-next-line dot-notation
                const errorStack = JSON.stringify(hint.originalException.stack)
                return /postMessage/.test(errorStack)
              }

              return false
            }
          }
        ]

        const shouldIgnoreError = ignoreErrorInBeforeSend.some(rule =>
          rule.check(event, hint)
        )
        return shouldIgnoreError ? null : event
      },
    release:"a21baf77b07df7892b8dcac059ef4e39c53d5eb7"
  }

which is still invalid due to beforeSend:beforeSend (event, hint) { part. Should be beforeSend:function (event, hint) {

Sorry, may I ask you how to run the test?
Because I used the same config you paste above, I still got the correct result.

My result:

  /* eslint-disable object-curly-spacing, quote-props, quotes, key-spacing, comma-spacing */
  const config = {
    dsn:"https:\u002F\u002Ffe8b7df6ea7042f69d7a97c66c2934f7@sentry.io.nuxt\u002F1429779",
    environment:"production",
    beforeSend:function (event, hint) {
                const ignoreErrorInBeforeSend = [
                    {
                        check: (event, hint) => {
                            // Ignore fbevent.js postMessage error in iOS device
                            if (hint.originalException.stack) {
                                // eslint-disable-next-line dot-notation
                                const errorStack = JSON.stringify(hint.originalException.stack);
                                return /postMessage/.test(errorStack);
                            }
                            return false;
                        }
                    }
                ];
                const shouldIgnoreError = ignoreErrorInBeforeSend.some(rule => rule.check(event, hint));
                return shouldIgnoreError ? null : event;
            },
    release:"b6fb6ef3295f635286d1df9db735a25f781000d5"
  }

I got correct beforeSend:function (event, hint) { result.

@rchl
Copy link
Member

rchl commented Jan 24, 2021

I'm testing on this repo itself with this change:

diff --git a/test/fixture/default/nuxt.config.js b/test/fixture/default/nuxt.config.js
index af2f3db..dc77268 100644
--- a/test/fixture/default/nuxt.config.js
+++ b/test/fixture/default/nuxt.config.js
@@ -17,7 +17,29 @@ const config = {
   ],
   sentry: {
     dsn: 'https://fe8b7df6ea7042f69d7a97c66c2934f7@sentry.io.nuxt/1429779',
-    config: {},
+    config: {
+      beforeSend (event, hint) {
+        const ignoreErrorInBeforeSend = [
+          {
+            check: (event, hint) => {
+              // Ignore fbevent.js postMessage error in iOS device
+              if (hint.originalException.stack) {
+                // eslint-disable-next-line dot-notation
+                const errorStack = JSON.stringify(hint.originalException.stack)
+                return /postMessage/.test(errorStack)
+              }
+
+              return false
+            }
+          }
+        ]
+
+        const shouldIgnoreError = ignoreErrorInBeforeSend.some(rule =>                                                                                                                                                                  
+          rule.check(event, hint)
+        )
+        return shouldIgnoreError ? null : event
+      }
+    },
     clientIntegrations: {
       // Integration from @Sentry/browser package.
       TryCatch: { eventTarget: false }

And then:

  • run yarn dev:fixture.
  • check the generated file in ./test/fixture/default/.nuxt/sentry.client.js

@charlie0228
Copy link
Contributor Author

Thanks!
After some testing, I found the issue which mat related to serializeFunction()

With beforeSend (event, hint) {, the serialized result will be beforeSend:beforeSend (event, hint) {
However, with beforeSend(event, hint) { (without space between function name and left bracket), the serialized result will be beforeSend:function (event, hint) {, which is the correct result.

@rchl
Copy link
Member

rchl commented Jan 25, 2021

Sounds like a bug in serializeFunction. Will you handle that on the Nuxt side?

@charlie0228
Copy link
Contributor Author

Sounds like a bug in serializeFunction. Will you handle that on the Nuxt side?

Maybe I can try it, otherwise, I have to use some hack to fix this issue.

@eljass
Copy link

eljass commented Feb 5, 2021

Any updates on this?

@rchl
Copy link
Member

rchl commented Feb 6, 2021

I found and fixed the Nuxt issue (nuxt/nuxt#8779) so once the Nuxt fix is integrated this fix will work in more (all?) cases.

This fix is good in itself though and I will merge it soon.

@rchl
Copy link
Member

rchl commented Feb 6, 2021

BTW. I haven't tried, but it should be possible to use the Runtime config to define beforeSend and avoid the original problem entirely.

@rchl rchl changed the title fix: update serialize function for sentry config fix: better handling of functions options in config Feb 7, 2021
@rchl rchl merged commit 341bed5 into nuxt-community:master Feb 7, 2021
@charlie0228
Copy link
Contributor Author

I found and fixed the Nuxt issue (nuxt/nuxt.js#8779) so once the Nuxt fix is integrated this fix will work in more (all?) cases.

This fix is good in itself though and I will merge it soon.

Thanks for helping!

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.

Incorrect serialize sentry config (beforeSend)
4 participants