Skip to content

Make file-system based routes optional for custom servers#914

Merged
arunoda merged 3 commits into
vercel:masterfrom
gcpantazis:making-page-routes-optional
May 27, 2017
Merged

Make file-system based routes optional for custom servers#914
arunoda merged 3 commits into
vercel:masterfrom
gcpantazis:making-page-routes-optional

Conversation

@gcpantazis
Copy link
Copy Markdown
Contributor

Related to #913, and making a custom routing setup more generally:

If you're using a custom server per the README and examples, you may not want to keep the file-system based routes. This commit simply puts that behind a configuration setting, so that you could turn those routes off by the following config.next.js:

module.exports = {
    useFileSystemPageRoutes: false,
}

If this is an acceptable solution I can expand the tests to cover it — let me know if that's worth doing. Thanks again for all the hard work!

Comment thread server/index.js Outdated
Copy link
Copy Markdown
Contributor

@timneutkens timneutkens Jan 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we can remove the !== false and add the default here https://github.com/zeit/next.js/blob/master/server/config.js#L6

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 🔥

@rauchg
Copy link
Copy Markdown
Member

rauchg commented Jan 28, 2017

Some people might even be interested in customizing the name of the directory, for example:

"pagesDirectory": "screens"

i wonder if pagesDirectory: null could then accomplish this? Or do you think it's too obscure? That way we have fewer settings ;)

@timneutkens
Copy link
Copy Markdown
Contributor

@rauchg that's interesting. pagesDirectory: null would be obscure. Though pagesDirectory: false might just be what we want in this situation 👍

@arunoda
Copy link
Copy Markdown
Contributor

arunoda commented Jan 29, 2017

I really like #913. But when you add a custom route it'll get the priority over the page based old routing. So, I don't think, we need a special flag to disable it.

@gcpantazis
Copy link
Copy Markdown
Contributor Author

@rauchg I dig the direction on using this to also customize pagesDirectory, agreed with @timneutkens on false as the value. I'll hook this up and update the tests.

@arunoda Hm, perhaps I'm misunderstanding but it seems like the custom server code needs to implement handle(req, res) to preserve the router's plumbing, which would persist all the file-system-based routes.

@arunoda
Copy link
Copy Markdown
Contributor

arunoda commented Jan 29, 2017

Since we do handle(req, res) at the end this is fine.
Also, it's legit for users to both types of routing at the same time.

@nkzawa
Copy link
Copy Markdown
Contributor

nkzawa commented Jan 29, 2017

I think the pagesDirectory option is not suitable for this purpose, since you might want to change the directory and want to disable file-system based routes at the same time.

As @arunoda mentioned, you don't need the flag if you define customs routes over file-system based ones, though it would be inconvenient.

Another way is to set the option as an argument of handle like:

handle(req, res, { fileSystemRoutes: false })

@gcpantazis
Copy link
Copy Markdown
Contributor Author

I think the pagesDirectory option is not suitable for this purpose, since you might want to change the directory and want to disable file-system based routes at the same time.

Ah, of course, that makes sense @nkzawa. I'm less sure if we want to add a secondary configuration into handle()'s arguments when we could just centralize it into next.config. (The need for it at all is probably a smell that the server router API needs to be expanded to allow for removing routes directly).

For now though, I'll proceed with adding the two options into next.config:

{
  pagesDirectory: 'foo', // default 'pages'
  useFileSystemPublicRoutes: false // default true
}

(Using the key useFileSystemPublicRoutes just since those routes would still exist for internal plumbing / your custom server routing code, they just wouldn't be exposed to users).

@gcpantazis gcpantazis changed the title Make file-system based routes optional for custom servers Make file-system based routes optional for custom servers, make pages directory name configurable Jan 30, 2017
@nkzawa
Copy link
Copy Markdown
Contributor

nkzawa commented Jan 30, 2017

Let's not include pagesDirectory on this PR. I think it's a different feature and we should discuss it anyway.

@gcpantazis
Copy link
Copy Markdown
Contributor Author

Sounds good @nkzawa, I'll split it into a separate PR and move that part of the discussion over there 👍

@gcpantazis gcpantazis force-pushed the making-page-routes-optional branch from 9da8170 to fa07f9d Compare January 30, 2017 17:37
@gcpantazis gcpantazis changed the title Make file-system based routes optional for custom servers, make pages directory name configurable Make file-system based routes optional for custom servers Jan 30, 2017
Copy link
Copy Markdown
Contributor

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@timneutkens
Copy link
Copy Markdown
Contributor

@nkzawa @arunoda leaving it to you to merge 👍

@timneutkens timneutkens added this to the 2.1 milestone Feb 27, 2017
@timneutkens timneutkens requested a review from arunoda May 27, 2017 11:54
@timneutkens
Copy link
Copy Markdown
Contributor

@arunoda can you check this one and merge if you want to take it 👍

@arunoda
Copy link
Copy Markdown
Contributor

arunoda commented May 27, 2017

Okay. I'm good with this.

@arunoda arunoda merged commit 2953a01 into vercel:master May 27, 2017
@sedubois
Copy link
Copy Markdown
Contributor

Should documentation be added?

@arunoda
Copy link
Copy Markdown
Contributor

arunoda commented May 28, 2017

@sedubois yeah! We can add it here in the options: https://github.com/zeit/next.js#custom-server-and-routing

brandonmp added a commit to brandonmp/next.js that referenced this pull request Dec 3, 2017
2 changes:

`passHref` - just added a cautionary note on the importance of `passHref`. We had a few days of no-href links on our site b/c we used a custom component instead of a raw `<a>` tag,  and Google bot wasn't crawling our links (confirmed in Google cache). Hurt our SEO a bit, so I thought it was worth noting.


`useFileSystemPublicRoutes` - this is mentioned in vercel#914 , but it doesn't appear any doc was actually added. We use `next-routes`, and we were serving all the files in `/pages/` in addition to their route patterns (ie duplicate content), which can be a pain w/ SEO and duplicate content.
timneutkens pushed a commit that referenced this pull request Dec 3, 2017
2 changes:

`passHref` - just added a cautionary note on the importance of `passHref`. We had a few days of no-href links on our site b/c we used a custom component instead of a raw `<a>` tag,  and Google bot wasn't crawling our links (confirmed in Google cache). Hurt our SEO a bit, so I thought it was worth noting.


`useFileSystemPublicRoutes` - this is mentioned in #914 , but it doesn't appear any doc was actually added. We use `next-routes`, and we were serving all the files in `/pages/` in addition to their route patterns (ie duplicate content), which can be a pain w/ SEO and duplicate content.
timneutkens pushed a commit to timneutkens/next.js that referenced this pull request Dec 4, 2017
2 changes:

`passHref` - just added a cautionary note on the importance of `passHref`. We had a few days of no-href links on our site b/c we used a custom component instead of a raw `<a>` tag,  and Google bot wasn't crawling our links (confirmed in Google cache). Hurt our SEO a bit, so I thought it was worth noting.


`useFileSystemPublicRoutes` - this is mentioned in vercel#914 , but it doesn't appear any doc was actually added. We use `next-routes`, and we were serving all the files in `/pages/` in addition to their route patterns (ie duplicate content), which can be a pain w/ SEO and duplicate content.
timneutkens added a commit that referenced this pull request Dec 5, 2017
* Add .jsx extension

* examples: add create-next-app (#3377)

* examples: add create-next-app

* fix with-typescript readme

* Upgrading with-flow example to the latest flow-bin ver. 0.59.0 (#3337)

For upgrading I used flow-upgrade module by https://yarnpkg.com/en/package/flow-upgrade

* doc'd fs-routing option & added note on `passHref` (#3384)

2 changes:

`passHref` - just added a cautionary note on the importance of `passHref`. We had a few days of no-href links on our site b/c we used a custom component instead of a raw `<a>` tag,  and Google bot wasn't crawling our links (confirmed in Google cache). Hurt our SEO a bit, so I thought it was worth noting.


`useFileSystemPublicRoutes` - this is mentioned in #914 , but it doesn't appear any doc was actually added. We use `next-routes`, and we were serving all the files in `/pages/` in addition to their route patterns (ie duplicate content), which can be a pain w/ SEO and duplicate content.

* fix typo in readme.md (#3385)

* Upgrade styled-jsx to v2.2.1 (#3358)

* Pulled encoding to top of head (#3214)

* Remove next.d.ts to use @types/next (#3297)

* Add with-mobx-state-tree example (#3179)

* Adapt with-mobx example for with-mobx-state-tree

* Remove unnecessary lastUpdate parameter to show off snapshot

* update readme

* make other.js more closely mimic index.js

* Upgrade styled-jsx to v2.2.1

Includes some bug fixes.

* Fix linting

* Make sure import that doesn’t end in .jsx works

* Move tests

* Show error when .js and .jsx both exist

* Remove .jsx when importing from ‘path.jsx’

* Fixes

* Get .jsx resolver back

* Revert "Get .jsx resolver back"

This reverts commit 6f76712.

* Revert "Revert "Get .jsx resolver back""

This reverts commit 69e592e.

* Add remove .jsx to preset

* Remove jsx resolver

* Revert "Remove jsx resolver"

This reverts commit 5e3ef1a.

* Revert "Revert "Remove jsx resolver""

This reverts commit 8248e50.

* Revert "Revert "Revert "Remove jsx resolver"""

This reverts commit 2a6d418.

* Make 1 component not use .jsx
@lock lock Bot locked as resolved and limited conversation to collaborators May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants