krakenjs / kraken-devtools

Development-time tools for kraken.js applications.
Other
40 stars 32 forks source link

support static html and paths with periods #38

Closed jasisk closed 9 years ago

jasisk commented 9 years ago

I'm not totally confident in our tests and I would qualify this as changing behavior so I recommend we do at least a minor bump and only after some more extensive testing (... maybe even writing more tests! GAPSP!).

This resolves implicit index.html. No super clean way to pull the exact resolution implementation from express (-> serve-static -> send) so this'll have to do. Though we may want to make it configurable down the line to match express.

Additionally, this doesn't conflate a period in a path with an extension like it used to.

Fixes #25.

jasisk commented 9 years ago

Build failing because of #37. Will kick off tests again after that gets merge.

jasisk commented 9 years ago

Failing due to node-sass (#46).

aredridel commented 9 years ago

Time to make a PR that merges both those branches?

jasisk commented 9 years ago

I rebased #46, waiting on Travis. Once that's in, I'll give this PR the same treatment.

aredridel commented 9 years ago

Disabling the node-sass tests if we're on a platform with module version 42 makes more sense than disabling iojs tests as a whole, I think. Less is left uncovered, and we can document that the break is known.

jasisk commented 9 years ago

Gonna add a few more tests that reflect things we've seen in the community.

grawk commented 9 years ago

Adding this test case surfaces an issue:

it.only('Removes trailing slash from a filename', function (done) {
    var app = testutil.createApp({
      copier: {
        module: './plugins/copier',
        files: '**/*'
      }
    });

    request(app)
      .get('/img/omgpath/index.html/')
      .expect(404)
      .end(done);
  });

Not sure what we should expect, but assume 404. Instead we get:

Error: ENOTDIR, open '/Users/medelman/src/kraken-devtools/test/fixtures/public/img/omgpath/index.html/index.html'
    at Error (native)
grawk commented 9 years ago

I tried the same test using the master branch and the test fails for different reasons. So, maybe we just don't support this particular use case?

grawk commented 9 years ago

After playing around with this for far too long, (basically to try and get the behavior of devtools to match that of kraken in production mode), I realized that there is a difference between what supertest and express are doing behind the scenes. If I use the new middleware.js as proposed in this PR, and run within an kraken application, I do get the desired 404 behavior with a route of the form /.../index.html/

:+1: on this PR from me.

aredridel commented 9 years ago

Works for me.