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

Tech debt: New Relic filtering: move up check for JS error filtering #14759

Merged
merged 3 commits into from May 3, 2017

Conversation

caleybrock
Copy link
Contributor

I moved the New Relic filtering outside of code-studio and into a separate entry point that happens earlier. I was hoping to add it earlier, but got some errors at each level earlier so I figured this was at least a good start.


//Included early to prevent JS Errors from being sent to New Relic
%script{src: minifiable_asset_path('js/layouts/application.js')}

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this shouldnt also be before essential/code-studio-common?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or alternatively, could we even just make it so that this ends up IN essential I wonder

Copy link
Contributor

Choose a reason for hiding this comment

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

Some errors (e.g. UncaughtException: duplicate formal argument e in very old versions of Firefox) appear to be coming from files we've imported but haven't run yet. Maybe there's a better workaround?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an example of IE8 throwing when it tries to parse application.js:

image

@caleybrock
Copy link
Contributor Author

@Bjvanminnen and @joshlory This now adds the new relic check to the build essential.js by creating an essential.js entry point that will be contanated with what gets build here: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/Gruntfile.js#L510

@joshlory
Copy link
Contributor

joshlory commented May 1, 2017

It looks like essential.js is included after application.js? (See: #14759 (comment).)

@caleybrock
Copy link
Contributor Author

The unsupportedBrowser check won't work before essential.js because my upstanding is essential loads what makes the rest of our JS work.
I'm not sure what the work around is for the Firefox bug, but I think the work around for IE 8 is to add back in the ruby file that filters out old IE versions. I think this work still helps, but it's challenging to catch everything.

@joshlory
Copy link
Contributor

joshlory commented May 2, 2017

Ah, I see — browser-detector.js doesn't import anything, but because of the webpack module stuff it won't work without webpackJsonp etc?

Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

I'd be tempted to put essential.js before application.js unless there's an order dependency there. Otherwise LGTM 👍 thanks for fixing!

@caleybrock
Copy link
Contributor Author

caleybrock commented May 2, 2017

@joshlory essential.js before application.js seems to work, although there is a bit of a concern that at some point something will get added to essential that requires jquery (right now application.js loads jquery). This should be obvious that it is broken, but maybe not obvious why.
I made a comment about the order in the haml file to help people if they run into this in the future. Hopefully, we'll start moving things out of application.js and into our apps pipeline and this won't be an issue.
After chatting with @Bjvanminnen we think this is worth it to make this more to reduce noise.

@caleybrock caleybrock merged commit 39be2ff into staging May 3, 2017
@caleybrock caleybrock deleted the move-new-relic-location branch May 3, 2017 17:33
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.

None yet

3 participants