Support optional trailing slash in /browse

modules/__tests__/browseDirectory-test.js

@@ -11,12 +11,10 @@ describe('A request to browse a directory', () => {
   describe('with no version specified', () => {
     it('redirects to the latest version', done => {
       request(server)
-        .get('/browse/react/')
+        .get('/browse/react')
         .end((err, res) => {
           expect(res.statusCode).toBe(302);
-          expect(res.headers.location).toMatch(
-            /\/browse\/react@\d+\.\d+\.\d+\//
-          );
+          expect(res.headers.location).toMatch(/\/browse\/react@\d+\.\d+\.\d+/);
           done();
         });
     });
@@ -25,7 +23,7 @@ describe('A request to browse a directory', () => {
   describe('when the directory exists', () => {
     it('returns an HTML page', done => {
       request(server)
-        .get('/browse/[email protected]/umd/')
+        .get('/browse/[email protected]/umd')
         .end((err, res) => {
           expect(res.statusCode).toBe(200);
           expect(res.headers['content-type']).toMatch(/\btext\/html\b/);
@@ -37,7 +35,7 @@ describe('A request to browse a directory', () => {
   describe('when the directory does not exist', () => {
     it('returns a 404 HTML page', done => {
       request(server)
-        .get('/browse/[email protected]/not-here/')
+        .get('/browse/[email protected]/not-here')
         .end((err, res) => {
           expect(res.statusCode).toBe(404);
           expect(res.headers['content-type']).toMatch(/\btext\/html\b/);
@@ -49,10 +47,10 @@ describe('A request to browse a directory', () => {
   describe('with invalid query params', () => {
     it('strips them from the query string', done => {
       request(server)
-        .get('/browse/[email protected]/umd/?invalid')
+        .get('/browse/[email protected]/umd?invalid')
         .end((err, res) => {
           expect(res.statusCode).toBe(302);
-          expect(res.headers.location).toEqual('/browse/[email protected]/umd/');
+          expect(res.headers.location).toEqual('/browse/[email protected]/umd');
           done();
         });
     });

modules/__tests__/browseRedirect-test.js

@@ -14,7 +14,7 @@ describe('A request with a trailing slash', () => {
         .get('/react/')
         .end((err, res) => {
           expect(res.statusCode).toBe(302);
-          expect(res.headers.location).toEqual('/browse/react/');
+          expect(res.headers.location).toEqual('/browse/react');
           done();
         });
     });
@@ -26,7 +26,19 @@ describe('A request with a trailing slash', () => {
         .get('/[email protected]/')
         .end((err, res) => {
           expect(res.statusCode).toBe(302);
-          expect(res.headers.location).toEqual('/browse/[email protected]/');
+          expect(res.headers.location).toEqual('/browse/[email protected]');
+          done();
+        });
+    });
+  });
+
+  describe('in /browse', () => {
+    it('redirects to the same url without trailing slash', done => {
+      request(server)
+        .get('/browse/[email protected]/')
+        .end((err, res) => {
+          expect(res.statusCode).toBe(302);
+          expect(res.headers.location).toEqual('/browse/[email protected]');
           done();
         });
     });

modules/__tests__/legacyURLs-test.js

@@ -33,7 +33,7 @@ describe('Legacy URLs', () => {
       .get('/[email protected]/umd/')
       .end((err, res) => {
         expect(res.statusCode).toBe(302);
-        expect(res.headers.location).toEqual('/browse/[email protected]/umd/');
+        expect(res.headers.location).toEqual('/browse/[email protected]/umd');
         done();
       });
   });

modules/actions/serveDirectoryBrowser.js

@@ -67,7 +67,7 @@ async function findMatchingEntries(stream, filename) {
 async function serveDirectoryBrowser(req, res) {
   const stream = await getPackage(req.packageName, req.packageVersion, req.log);
 
-  const filename = req.filename.slice(0, -1) || '/';
+  const filename = req.filename || '/';
   const entries = await findMatchingEntries(stream, filename);
 
   if (Object.keys(entries).length === 0) {

modules/client/browse/App.js

@@ -141,15 +141,19 @@ export default function App({
     );
   }
 
+  // We're on the package root directory when filename is an empty string
+  const isRoot = !filename;
+
   const breadcrumbs = [];
+  const rootUrl = `/browse/${packageName}@${packageVersion}`;
 
-  if (filename === '/') {
+  if (isRoot) {
     breadcrumbs.push(packageName);
   } else {
-    let url = `/browse/${packageName}@${packageVersion}`;
+    let url = rootUrl;
 
     breadcrumbs.push(
-      <a href={`${url}/`} css={linkStyle}>
+      <a href={url} css={linkStyle}>
         {packageName}
       </a>
     );
@@ -164,7 +168,7 @@ export default function App({
     segments.forEach(segment => {
       url += `/${segment}`;
       breadcrumbs.push(
-        <a href={`${url}/`} css={linkStyle}>
+        <a href={url} css={linkStyle}>
           {segment}
         </a>
       );
@@ -298,7 +302,11 @@ export default function App({
             }}
           >
             {target.type === 'directory' ? (
-              <DirectoryViewer path={target.path} details={target.details} />
+              <DirectoryViewer
+                path={rootUrl + target.path}
+                isRoot={isRoot}
+                details={target.details}
+              />
             ) : target.type === 'file' ? (
               <FileViewer path={target.path} details={target.details} />
             ) : null}

modules/client/browse/DirectoryViewer.js

@@ -5,6 +5,7 @@ import VisuallyHidden from '@reach/visually-hidden';
 import sortBy from 'sort-by';
 
 import { formatBytes } from '../utils/format.js';
+import { join, basename, dirname } from '../utils/path.js';
 
 import { DirectoryIcon, CodeFileIcon } from './Icons.js';
 
@@ -44,19 +45,16 @@ const typeCellStyle = {
   }
 };
 
-function getRelName(path, base) {
-  return path.substr(base.length > 1 ? base.length + 1 : 1);
-}
-
-export default function DirectoryViewer({ path, details: entries }) {
+export default function DirectoryViewer({ path, isRoot, details: entries }) {
+  const parentPath = dirname(path);
   const rows = [];
 
-  if (path !== '/') {
+  if (!isRoot) {
     rows.push(
       <tr key="..">
         <td css={iconCellStyle} />
         <td css={tableCellStyle}>
-          <a title="Parent directory" href="../" css={linkStyle}>
+          <a title="Parent directory" href={parentPath} css={linkStyle}>
             ..
           </a>
         </td>
@@ -83,8 +81,8 @@ export default function DirectoryViewer({ path, details: entries }) {
   );
 
   subdirs.sort(sortBy('path')).forEach(({ path: dirname }) => {
-    const relName = getRelName(dirname, path);
-    const href = relName + '/';
+    const relName = basename(dirname);
+    const href = join(path, relName);
 
     rows.push(
       <tr key={relName}>
@@ -105,8 +103,8 @@ export default function DirectoryViewer({ path, details: entries }) {
   files
     .sort(sortBy('path'))
     .forEach(({ path: filename, size, contentType }) => {
-      const relName = getRelName(filename, path);
-      const href = relName;
+      const relName = basename(filename);
+      const href = join(path, relName);
 
       rows.push(
         <tr key={relName}>
@@ -174,6 +172,7 @@ export default function DirectoryViewer({ path, details: entries }) {
 if (process.env.NODE_ENV !== 'production') {
   DirectoryViewer.propTypes = {
     path: PropTypes.string.isRequired,
+    isRoot: PropTypes.bool.isRequired,
     details: PropTypes.objectOf(
       PropTypes.shape({
         path: PropTypes.string.isRequired,

modules/client/utils/path.js

@@ -0,0 +1,22 @@
+export function join(...paths) {
+  return (
+    paths
+      .join('/')
+      .replace(/\/+/g, '/')
+      .replace(/\/$/, '') || '.'
+  );
+}
+
+export function basename(path) {
+  return path.split('/').slice(-1);
+}
+
+export function dirname(path) {
+  return (
+    path
+      .replace(/\/$/, '')
+      .split('/')
+      .slice(0, -1)
+      .join('/') || '.'
+  );
+}

modules/createServer.js

@@ -20,6 +20,7 @@ import validateFilename from './middleware/validateFilename.js';
 import validatePackagePathname from './middleware/validatePackagePathname.js';
 import validatePackageName from './middleware/validatePackageName.js';
 import validatePackageVersion from './middleware/validatePackageVersion.js';
+import validateFileType from './middleware/validateFileType.js';
 
 function createApp(callback) {
   const app = express();
@@ -56,24 +57,20 @@ export default function createServer() {
     app.use(
       '/browse',
       createApp(app => {
-        app.enable('strict routing');
-
-        app.get(
-          '*/',
-          noQuery(),
-          validatePackagePathname,
-          validatePackageName,
-          validatePackageVersion,
-          serveDirectoryBrowser
-        );
-
         app.get(
           '*',
           noQuery(),
           validatePackagePathname,
           validatePackageName,
           validatePackageVersion,
-          serveFileBrowser
+          validateFileType,
+          (req, res) => {
+            if (req.type === 'directory') {
+              return serveDirectoryBrowser(req, res);
+            }
+
+            return serveFileBrowser(req, res);
+          }
         );
       })
     );
@@ -139,7 +136,7 @@ export default function createServer() {
 
     // Send old */ requests to the new /browse UI.
     app.get('*/', (req, res) => {
-      res.redirect(302, '/browse' + req.url);
+      res.redirect(302, '/browse' + req.url.slice(0, -1));
     });
 
     app.get(

modules/middleware/validateFileType.js

@@ -0,0 +1,72 @@
+import tar from 'tar-stream';
+
+import asyncHandler from '../utils/asyncHandler.js';
+import { getPackage } from '../utils/npm.js';
+
+async function findEntryType(stream, filename) {
+  // filename = /some/file/name.js
+  return new Promise((accept, reject) => {
+    let hasFoundFile = false;
+
+    stream
+      .pipe(tar.extract())
+      .on('error', reject)
+      .on('entry', async (header, stream, next) => {
+        if (hasFoundFile) {
+          next();
+        }
+
+        const entry = {
+          // Most packages have header names that look like `package/index.js`
+          // so we shorten that to just `/index.js` here. A few packages use a
+          // prefix other than `package/`. e.g. the firebase package uses the
+          // `firebase_npm/` prefix. So we just strip the first dir name.
+          path: header.name.replace(/^[^/]+\/?/, '/'),
+          type: header.type
+        };
+
+        // Ignore files that don't match the name.
+        if (entry.type !== 'file' || entry.path !== filename) {
+          stream.resume();
+          stream.on('end', next);
+        } else {
+          hasFoundFile = true;
+
+          next();
+        }
+      })
+      .on('finish', () => {
+        accept(hasFoundFile ? 'file' : 'directory');
+      });
+  });
+}
+
+async function validateFileType(req, res, next) {
+  if (!req.filename) {
+    req.type = 'directory';
+  } else {
+    const stream = await getPackage(
+      req.packageName,
+      req.packageVersion,
+      req.log
+    );
+
+    const entryType = await findEntryType(stream, req.filename);
+
+    req.type = entryType;
+  }
+
+  if (req.type === 'directory' && req.filename.slice(-1) === '/') {
+    // If there is a trailing slash, redirect to the url without it
+    return res
+      .set({
+        'Cache-Control': 'public, max-age=31536000', // 1 year
+        'Cache-Tag': 'redirect, file-type-redirect'
+      })
+      .redirect(302, req.originalUrl.slice(0, -1));
+  }
+
+  next();
+}
+
+export default asyncHandler(validateFileType);