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

[BUGFIX] Don't override the request's path #3160

Merged
merged 1 commit into from
Feb 23, 2015
Merged

[BUGFIX] Don't override the request's path #3160

merged 1 commit into from
Feb 23, 2015

Conversation

dmathieu
Copy link

I am writing an add-on which needs to get access to some URLs server-side (to perform OAuth authentication).
The thing is, the request's path is being rewritten to display the default file content.

Displaying the default file when nothing else can be rendered makes sense. But overriding the path and preventing middlewares from reading doesn't.

@dmathieu dmathieu changed the title Don't override the request's path [BUGFIX] Don't override the request's path Jan 30, 2015
@chadhietala
Copy link
Member

Restarting Appveyor.

@stefanpenner
Copy link
Contributor

tests?

@dmathieu
Copy link
Author

@stefanpenner: I have nothing against writing tests. Unfortunately, after spending a bit of time on it, I feel like writing this test would need refactoring all of add-ons.
I intended to write something like this:

it('allows addons to render on a path', function(done) {
  return subject.start({
    host:  '0.0.0.0',
    port: '1337',
    baseURL: '/'
  }).then(function() {
    request(subject.app)
      .get('/addon.foobar')
      .set('accept', 'text/html')
      .expect(200)
      .expect(function(res) {
        expect(res.text).to.equal('rendered by addon');
      })
      .end(function(err) {
        if (err) {
          return done(err);
        }
        done();
       });
  });
});

Unfortunately, at line 561, the add-ons test resets the initializeAddons method, which removes all built-in add-ons.
I believe this prevents from properly testing them, as a built-in add-on can introduce something breaking other ones further in the stack (which is the case here).

I took a quick look at the possibility of not resetting that method, but it looks like that would require quite a lot of setup.

Where/how would you add this test?

stefanpenner added a commit that referenced this pull request Feb 23, 2015
[BUGFIX] Don't override the request's path
@stefanpenner stefanpenner merged commit 5cddc65 into ember-cli:master Feb 23, 2015
@dmathieu dmathieu deleted the no_override_path branch February 24, 2015 09:41
@dmathieu
Copy link
Author

Thank you @stefanpenner

@stefanpenner
Copy link
Contributor

Thank you

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