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(ember): resolve config path from app root #3240

Conversation

alexlafroscia
Copy link
Contributor

This should aid in resolving the app configuration from the right location. Previously, it was calling require with a relative path from the index.js file of the Sentry addon itself. This broke trying to actually load the configuration file in other addons that might include this as a dependency.

By resolving the config path relative to the app root first, we can correctly load the file even when @sentry/ember is used within another addon.


Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@alexlafroscia
Copy link
Contributor Author

I'm getting a related issue to this when having @sentry/ember installed in one of my apps, rather than an add-on

  • Importing the actual config fails because the app doesn't have app.options.configPath defined
  • Defaults to an empty object, so the config has no sentry property
  • Configured application fails to boot due to an exception because the sentry property doesn't exist

Our app configuration is pretty conventional, so I'm wondering how this isn't broken for everyone...

I'm going to look into other ways the path to the app's config file can be looked up so that it can be reliably found.

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Feb 8, 2021

Since app.options.configPath is not defined when @sentry/ember is included inside an Ember App (rather than an addon, where it is defined by the test "dummy app"), providing a fallback to the conventional relative path of config/environment fixes the issue I'm seeing!

@Gorzas
Copy link

Gorzas commented Feb 9, 2021

@alexlafroscia testing it in my webapp, I've seen that using app.project.configPath()gets the actual path in Ember v3.12. Have you tried that one or there's something I'm missing?

@Gorzas Gorzas mentioned this pull request Feb 9, 2021
9 tasks
@alexlafroscia
Copy link
Contributor Author

Oh that's interesting, I haven't tried that yet! Unfortunately none of this is very well documented on the Ember CLI side of things, if at all.

I'll check that out in both the Ember app and addon that I've been trying to integrate this library into and see if that works reliably. I had poked around on app.options and app.project and didn't notice it, but it's very possible I just missed something!

This should aid in resolving the app configuration from the right location.

Previously, it was calling `require` with a relative path from the `index.js` file of the Sentry addon itself. This broke trying to actually load the configuration file in _other_ apps or addons that might include this as a dependency.

The Ember addon API exposes the configuration path for a given app or addon through the `project.configPath()` method, which we can use instead to look up the location of the project's `config/environment.js` file.
@alexlafroscia
Copy link
Contributor Author

Great call-out @Gorzas, that API is definitely the way to go! I confirmed locally that it works both when @sentry/ember is a dependency of an addon or an app; in both cases, app.project.configPath() returns the expected value!

I rebased this PR to simplify things and just use that.

@kamilogorek
Copy link
Contributor

cc @k-fish

@k-fish
Copy link
Member

k-fish commented Feb 17, 2021

@alexlafroscia hey sorry, I missed this PR but I have already put and merged a PR (#3246) that should hopefully fix this on both sides of Embroider and the classic build system, instead of trying to pull from app in a different hook, it sets it inside the config addon hook.

Anyway, I only have the dummy test app to try it out with, if you'd like to pull sentry-javascript master and test out the change with your app to confirm, that would be great 👍

@kamilogorek
Copy link
Contributor

if you'd like to pull sentry-javascript master and test out the change with your app to confirm, that would be great 👍

Or you can use 6.2.0 which includes this change already :)

@alexlafroscia
Copy link
Contributor Author

That'll be much easier @kamilogorek ! I'll do that now

@alexlafroscia
Copy link
Contributor Author

Seems like the 6.2.0 release fixes things too! Thanks @k-fish !

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

4 participants