File system routes #96

IlyaSemenov · 2018-06-03T18:10:34Z

This PR fixes #68. It may look huge, but the most part of the PR is the new example directory and tests.

If there is a directory pages, its contents will be used to construct a router. This router will be injected by a plugin into entry object.
Dynamic routes can be created with ~~_param.vue or~~ [param].vue (or :param.vue, although not on Windows).
Child routes are created if there are both some.vue and some/ in the same directory. If there's only some/, it also works, but adds prefixed routes to common list.
User-provided entry is no more required. Ream will work with nothing but pages/*.vue (same as Nuxt). If entry is there, it is used. If there's no pages (and no router that came from entry), an exception will be thrown like it used to be (but better as it is more descriptive).
It is possible to disable file system routes, change pages to something else, or change supported extensions. (I am also thinking about other options, e.g. trailingSlashes: true, or a custom filter function.)
It is possible to disable entrypoint. (Not that it's really needed, but it came logically.)

I had to make some small changes in create-app-template.js to be able to allow plugins to override the router, and to make entry point optional, but most of the code went into a plugin.

The default naming pages (not routes and not views) was chosen because I believe it reflects better what users are putting there — pages that will show on their site. (Routes are URL matching rules, and views are from MVC world where there are also controllers in front — with fsRoutes, users don't create neither of these.)

write-routes.js is specifically made to be reusable. In my project, I need to have not one but two file system routers (for project.com and for app.project.com). It is now possible with a small ad-hoc plugin that calls write-routes.js twice.

routes-template.js is decoupled from Ream API for easier testing.

TODO: - Recursive routes - Dynamic routes - Update documentation - Make entry file optional

egoist · 2018-06-04T09:32:09Z

lib/plugins/fs-routes/router-template.js

+  // Nuxt-compatible syntax: _item_id.vue
+  // egoist suggested syntax: [item_id].vue
+  return path.replace(/^_/, ':').replace(/\[(.*?)\]/g, ':$1')
+}


I prefer using [username].vue syntax and ignoring paths starting with an underscore _, when I was using Nuxt.js I always want to put some page-related components in pages but don't know how to exclude them.

Done. _* and .* are excluded, [username].vue is the only supported syntax.

egoist · 2018-06-04T09:39:54Z

lib/plugins/fs-routes/router-template.js

+// catalog/index.vue -> /catalog
+// catalog/specials.vue -> /catalog/specials
+
+async function collectRoutes({ dir, componentPrefix, parentPath }) {


maybe use fast-glob to simplify this, and chokidar to add watch functionality.

Agreed with chokidar, but I don't think fast-glob will be simpler. The reasons being:

non-uniform directory recursion (a directory may either generate child routes with cleared path prefix, or extend path prefix and append to the parent routes)

per-directory sorting

egoist · 2018-06-04T09:42:22Z

app/create-app-template.js

+          }export "router" in ${api.options.entry || `entry file`}!`
+        )})
+      }
+    }


I'm thinking about creating a default router instance here if entry.router is not provided, and use router.addRoutes to add routes in the fs-routes plugin.

I reworked it the way you described. One of the benefits it's that it's now possible to merge entry routes with fs routes (with former taking precedence).

However, it's now not possible to detect a misconfigured router in create-app-template.js, as router doesn't expose the full set of routes (you can't rely on router.options.routes as it only contains the initial set of routes and not the ones added by addRoutes):
https://github.com/vuejs/vue-router/blob/4cb6699ecc4453a992429b28f32f4add3e2680b6/src/create-matcher.js#L171

Thoughts?

egoist · 2018-06-04T09:44:37Z

app/create-app-template.js

+      require.resolve(api.resolveBaseDir(api.options.entry))
+      entryExists = true
+    } catch (err) {}
+  }


we can use require.context in the runtime code to check if there's entry file so that the app will be properly reloaded when you add an entry file.

I added hot reload for the entry with require.context.

Since it requires string literals at compile time, the code is a bit verbose. I guess it could be moved into a reusable code generation function eventually (so the code becomes something like const _entry = ${importOptional(api.baseDir, api.options.entry)}, but I'm not entirely sure where you'd like it to reside.

egoist · 2018-06-05T09:43:16Z

lib/plugins/fs-routes/routes-template.js

+  return `export default ${renderRoutes(routes)}`
+}
+
+function renderRoutes(routes) {


if you have time, we can start adding some unit tests at lib/plugins/fs-routes/__test__/routes.test.js using jest, otherwise I'll do it later.

@egoist I added tests.

IlyaSemenov · 2018-06-06T11:33:11Z

@egoist I addressed everything that you've mentioned. The only problem that I see is that we lost "You must export "router" in entry file!" message as it's not now possible to determine if router has any added routes or not.

What I think is that in dev mode, for / URL only, if it gives 404, we may add this to the error text: "You must add pages/*.vue or export "router" in entry file."

Add regression test for empty pages dir.

IlyaSemenov · 2018-06-06T18:44:01Z

OK so what bothers me is naming.

fsRoutes: {
  path: 'pages',
  basePath: '/',
  match: /\.(vue|js)$/i
}

How can we distinguish between path as for path in base dir, and basePath as for base route path? Rename path to dir or baseDir?

UPDATE: renamed as baseDir, and updated internal naming correspondingly.

egoist · 2018-06-07T10:15:24Z

app/create-app-template.js

+    if (entry.extendRootOptions) {
+      entry.extendRootOptions(rootOptions)
+    }
+


why after enhanceAppFiles 🤔

I was thinking that user provided entry callback should be called last (to have chance to correct all previous plugin-imposed setup). This change is not needed for the PR. If you dislike it I can revert.

egoist · 2018-06-07T10:17:27Z

app/enhance-app.js

+        {__DEV__ &&
+          error.code === 404 &&
+          error.errorPath === '/' && (
+            <p>You must create pages/*.vue or export "router" in entry file!</p>


is this message necessary? I mean it's obviously missing router or pages when there's 404 error

Absolutely not. Problem is, your original message from https://github.com/ream/ream/blob/a66d664477f4519b9d362893dd90c36d3184fe82/app/create-app-template.js#L62 is gone (as router is always instantiated), so I figured I would restore it somewhere else.

Also note that it's only printed for /. In dev mode, this most probably means a misconfigured install.

egoist · 2018-06-07T11:01:25Z

lib/index.js

@@ -49,6 +49,11 @@ class Ream extends Event {
    this.options = merge(
      {
        entry: 'index.js',
+        fsRoutes: {
+          baseDir: 'pages',
+          basePath: '/',


is this the same thing as router.options.base? maybe we can get it from the router instead.

I don't think so; router.options.base is runtime and this basePath is compile-time (it's imprinted in .ream/routes.js). Unless we go with some routes post-processing in inject.js it's not possible to achieve.

Also, I don't think it's really needed. The defaults are good for 95% use cases; for the rest additional flexibility wouldn't hurt.

Moreover, from what I believe router.options.base is automatically applied in runtime to all routes. So it's not the same thing. For example, I may set router.options.base to /foo and fsRoutes.baseDir to /bar and then pages/baz.vue will open at /foo/bar/baz.

egoist · 2018-06-14T13:35:06Z

It's weird that on your branch my computer runs out memory real fast..

Step to reproduce:

cd ream && yarn link
cd examples/fs-routes
rm node_modules && yarn link ream
yarn dev

Open http://localhost:4000

Errors:

2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-14 21:24 node[4192] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)

It works fine on master

IlyaSemenov · 2018-06-16T07:48:39Z

Hmmm yes I rm -rf and reinstalled everything with Node 10.4.1 and I now get that problem, too. Investigating.

5688c70 is the first bad commit (the require.context magic to watch for the entry file in run time). That's odd as it used to work. Investigating further.

IlyaSemenov · 2018-06-16T09:35:36Z

OK, so it crashes because of this line:

  const context = require.context(
    "/Users/semenov/work/github/patches/ream/examples/fs-routes",
    false,
    /^\\.\\/index\\.js$/
  )

Simply creating a webpack context (not even resolving it) makes it fail. Looks like it's specific to OS X, and happens when there's a lot of files in a directory. (Lots of similar reports: gruntjs/grunt-contrib-watch#75, expo/create-react-native-app#281, etc. etc.)

It didn't happen to me before because I didn't use yarn link but rather ../../bin/cli.js dev to test. Thus, I didn't have node_modules in examples/fs-routes. It also doesn't happen on Ubuntu 18.04

I tried upgrading everything with rm yarn.lock && yarn but it didn't help.

Unless there's some workaround, I guess the only options are either to disable the auto-discovery of the entry file, or make it required again.

IlyaSemenov · 2018-06-16T15:50:02Z

I reproduced the crash in a standalone project, with this:

const context = require.context(
    "/Users/semenov/work/github/patches/ream/examples/fs-routes",
    false,
    /^\.\/index\.js$/
  )

setTimeout(() => {}, 2**30)

this goes to src/index.js, then run it with backpack, and it crashes the same way:

2018-06-16 22:48 node[72024] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-16 22:48 node[72024] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
2018-06-16 22:48 node[72024] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-21)
...

IlyaSemenov · 2018-06-16T15:55:13Z

The problem only occurs when using yarn link specifically. It does not happen when using normal yarn (with whatever amount of files is there in node_modules).

Related webpack issue: webpack/watchpack#45

egoist · 2018-06-17T13:14:22Z

there's a dirty workaround: 😅

if there's an entry file, we create a file called .ream/entry-mirror.js which simply exports the actual entry file. Then in create-app-template.js we can check if the entry-mirror.js exists instead.

IlyaSemenov · 2018-06-18T06:11:37Z

I don't see how it's different from simply checking if the actual entry exists in create-app-template.js. It will not work with HMR, will it?

What I'm thinking we may come up with a webpack resolver plugin which falls back unresolved #app-entry to ream/app/default-entry.js. But I didn't manage to come up with a working prototype quickly. I'll see to that again in a couple of days.

IlyaSemenov · 2018-06-23T10:37:56Z

I managed to create an enhanced-resolve (webpack resolver) plugin that iterates over a list of aliased modules and resolves import with the first successful match.

Unfortunately, that does not directly solve the problem. Once it resolves to say #app/entry (the fallback alias that I put after api.resolveBaseDir(api.options.entry)) it's stuck to it. So when a user puts the entry file in place, no hot reload happens.

IlyaSemenov · 2018-06-23T12:05:22Z

@egoist I reworked entry file watching with chokidar. Do you think this is enough to merge?

IlyaSemenov · 2018-06-25T07:44:52Z

FWIW I believe we may also watch ream.config.js the similar way ;) But it's surely beyond the scope of this issue.

IlyaSemenov added 5 commits June 3, 2018 16:38

fs-routes working prototype

6eb544f

TODO: - Recursive routes - Dynamic routes - Update documentation - Make entry file optional

Make entry file optional

15e3388

Allow to override pages path

572a77b

Recursive and dynamic fs-routes

27f2f1f

Fix formatting

b08c623

egoist reviewed Jun 4, 2018

View reviewed changes

IlyaSemenov added 2 commits June 5, 2018 15:30

Simplified fs-routes example

164f271

Merge entry routes with fs routes

eda1862

egoist reviewed Jun 5, 2018

View reviewed changes

IlyaSemenov added 3 commits June 5, 2018 18:01

Refactor fs-routes files collector

bd4ee22

Use require.context for hot reload of ream entry

5688c70

Add tests for fs-routes

b6e518c

IlyaSemenov force-pushed the features/fs-routes branch from a40e6b4 to b6e518c Compare June 5, 2018 17:49

IlyaSemenov added 3 commits June 6, 2018 00:59

Simplify fs-routes test runner

f60440e

Update fs-routes expected routes naming and format

a83e367

Watch pages path and recreate routes

222315d

Remove stale debug code

92a3227

IlyaSemenov force-pushed the features/fs-routes branch from 2865ef9 to 92a3227 Compare June 6, 2018 14:14

IlyaSemenov added 4 commits June 6, 2018 21:16

tweaks

ca88c69

Handle non-existent pages directory

0d46331

Decouple write-routes.js from ream API.

11737bc

Add regression test for empty pages dir.

Show explanatory message when / gives 404 in dev mode

56b846a

Change parameters naming

7c81524

egoist reviewed Jun 7, 2018

View reviewed changes

adjust naming

2e3357d

egoist reviewed Jun 7, 2018

View reviewed changes

fs.pathExists instead of fs.exists

3911d61

Fix create-app-template when api.options.entry is disabled

94943b8

Hot reload for added entry file with chokidar

2b354a0

IlyaSemenov mentioned this pull request Jun 23, 2018

Remove #create-app alias #102

Merged

Rollback this.resolveOutDir change

bab626b

egoist added 2 commits August 7, 2018 13:59

Merge branch 'master' into features/fs-routes

18ba879

tweaks

74ef009

egoist merged commit 48c3333 into ream:master Aug 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File system routes #96

File system routes #96

IlyaSemenov commented Jun 3, 2018 •

edited

Loading

egoist Jun 4, 2018

IlyaSemenov Jun 5, 2018

egoist Jun 4, 2018

IlyaSemenov Jun 4, 2018

egoist Jun 4, 2018 •

edited

Loading

IlyaSemenov Jun 5, 2018

egoist Jun 4, 2018 •

edited

Loading

IlyaSemenov Jun 5, 2018

egoist Jun 5, 2018 •

edited

Loading

IlyaSemenov Jun 5, 2018

IlyaSemenov commented Jun 6, 2018 •

edited

Loading

IlyaSemenov commented Jun 6, 2018 •

edited

Loading

egoist Jun 7, 2018

IlyaSemenov Jun 7, 2018

egoist Jun 7, 2018

IlyaSemenov Jun 7, 2018

IlyaSemenov Jun 7, 2018

egoist Jun 7, 2018

IlyaSemenov Jun 7, 2018

IlyaSemenov Jun 7, 2018

egoist commented Jun 14, 2018 •

edited

Loading

IlyaSemenov commented Jun 16, 2018 •

edited

Loading

IlyaSemenov commented Jun 16, 2018

IlyaSemenov commented Jun 16, 2018

IlyaSemenov commented Jun 16, 2018

egoist commented Jun 17, 2018

IlyaSemenov commented Jun 18, 2018

IlyaSemenov commented Jun 23, 2018

IlyaSemenov commented Jun 23, 2018

IlyaSemenov commented Jun 25, 2018

File system routes #96

File system routes #96

Conversation

IlyaSemenov commented Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egoist Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egoist Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egoist Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IlyaSemenov commented Jun 6, 2018 • edited Loading

IlyaSemenov commented Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egoist commented Jun 14, 2018 • edited Loading

IlyaSemenov commented Jun 16, 2018 • edited Loading

IlyaSemenov commented Jun 16, 2018

IlyaSemenov commented Jun 16, 2018

IlyaSemenov commented Jun 16, 2018

egoist commented Jun 17, 2018

IlyaSemenov commented Jun 18, 2018

IlyaSemenov commented Jun 23, 2018

IlyaSemenov commented Jun 23, 2018

IlyaSemenov commented Jun 25, 2018

IlyaSemenov commented Jun 3, 2018 •

edited

Loading

egoist Jun 4, 2018 •

edited

Loading

egoist Jun 4, 2018 •

edited

Loading

egoist Jun 5, 2018 •

edited

Loading

IlyaSemenov commented Jun 6, 2018 •

edited

Loading

IlyaSemenov commented Jun 6, 2018 •

edited

Loading

egoist commented Jun 14, 2018 •

edited

Loading

IlyaSemenov commented Jun 16, 2018 •

edited

Loading