Dependency updates and decouple the addon from the rollbar library #108
Conversation
|
@paulcwatts - huge thanks for this PR. I've been meaning to investigate whether it would be possible to avoid the hardcoded snippet, both for increased flexibility and lower maintenance burden, so I'm super excited about this. A couple thoughts:
If that file location changes, we might have sudden failures on CIs as we try to bring in a file that no longer exists. I think the prerequisite for this approach would be (a) some confirmation that the snippet file location is at least under semver guarantees (i.e. the rollbar team will bump majors if they move it), and (b) this line needs to change to include the current major version number. I'd rather err on the side of being too slow to update this lib to match Rollbar's latest, rather than introducing the potential for sudden, unexpected breakages in the future cause by a seemingly innocuous change by the Rollbar team. |
This is basically a rewrite that: 1. Removes the included snippet, and instead includes the snippet from the rollbar node library. 2. Use a default addon blueprint to add the rollbar package to the host app. This allows the host to update the rollbar package without having to upgrade ember-cli-rollbar. 3. Use ember-cli-node-assets to include the vendor file. 4. Use broccoli-file-writer to generate the config. This does not yet support Fastboot -- that's coming up.
Update to be consistent with the code style.
1. Using the new Fastboot standards, don't run the snippet if Fastboot isn't defined, because the snippet requires 'document'. 2. Reference Rollbar using window.Rollbar in the vendor shim, because 'Rollbar' is undefined in Fastboot. 3. Add a very basic test to test these. Note this doesn't do much besides ensure that the app doesn't break when Fastboot is enabled. The vendor shim won't work, and since window.Rollbar won't be defined, it won't actually forward any of the calls.
paulcwatts
force-pushed
the
update-rollbar
branch
from
9b6b9c2
to
9687144
Compare
|
@davewasmer I rebased the branch so it's only the implementation changes. Aside from the unnecessary files I found in #110, I also noticed that master no longer includes With regard to the Rollbar library, it's been isomorphic since version 2.0. As far as dealing with newer versions, it appears from their changelog they support semver. We can include a major version in the blueprint. However, it should be noted that with this new implementation, the host app decides what version to use, not the addon. The host app can always include a newer version its package.json that may be incompatible with this addon. In case there are breaking changes, it's possible to use something like ember-cli-version-checker to support both. Is there a reason to continue to support Node 4? |
|
By the way, I have additional changes in the hopper: I didn't want to include it in this PR to limit the changes, but this change adds support for |
|
Hi @davewasmer, have you had a chance to look at the updates? The build is failing on Fastboot with Node 4, that's the reason why I'm asking whether Node 4 needs to be supported. |
|
@paulcwatts it's not entirely clear to me from the file changes so I thought I would ask: does this implementation enable rollbar reporting in the fastboot Node environment or does it just disable/ignore it so that it doesn't crash the application? |
|
@mansona It's the latter. I believe you still need to enable rollbar using their Express middleware, but to be honest I haven't done much Fastboot dev so I'm not exactly sure how Fastboot handles errors. It's possible the change to handle |
|
I just took a quick look and it doesn't look like it will work in the fastboot environment, but yes it will be much simpler to make it work. The main problem that I have is that Rollbar is not technically isomorphic (I won't get into how that is a pipe-dream) because the one repo has many different build targets that come off it. So the asset you get in the frontend is not the same you get in the backend. It adds a bit of complication but I'm sure we could figure it out. I would recommend that this PR get merged, because it is a nice simplification, and then any work I was planning to do could be done on top of it 👍 |
|
Thanks @mansona. I can disable the fastboot tests (or just allow them to fail on Travis) and tackle that at some later point. |
|
sorry @paulcwatts I thought you had got this to work by disabling it in the fastboot environment 🤔 Just to be clear: I think that it should be disabled in fastboot for now and we can add an implementation that works in fastboot later after your PR is merged. Also these are just my thoughts 😂 this is all really up to @davewasmer 👍 |
They don't work with Node 4, and Fastboot doesn't really work to begin with.
|
This PR is now 2 months old. Are there still open questions? What needs to be done to get this merged? |
|
My position still stands, I think @davewasmer should merge this 👍 |
|
By the way, I've updated my fork with this change and a bunch of other things (including working Fastboot error notifications) here: https://github.com/paulcwatts/ember-cli-rollbar I've also published this fork here: https://www.npmjs.com/package/ember-cli-rollbar2 |
|
Using @paulcwatts fork and it work like a charm! Please, merge it! |
I realize this is a large PR. I just wanted to get the conversation started.
The first part of this is an update to the latest dependencies, including ember-cli and updating to the latest ember-cli blueprint.
The second is a rewrite of the addon functionality that:
I've also updated the Fastboot functionality to be compliant with the Fastboot 1.0 changes, although I haven't had much ability to fully test that (none of my current projects use Fastboot).
Despite the lines of code change, it actually simplifies the addon significantly because it doesn't have to update the snippet and uses external addons to include files from node_modules.
I've been using this fork in an an internal project of mine for a few months now.