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

Programmatic API #310

Merged
merged 22 commits into from
Dec 16, 2016
Merged

Programmatic API #310

merged 22 commits into from
Dec 16, 2016

Conversation

nkzawa
Copy link
Contributor

@nkzawa nkzawa commented Nov 30, 2016

Fixes #291

  • support hot-code reloading even on custom routes
  • enable to use high and low level APIs depends on your needs

Ideas are basically the same with #291, but some APIs are a bit different.

API

next({ dir = '.', dev = false } = {})

const next from 'next'
const app = next()
  • app.render(req, res, pathname, query) - renders pathname and automatically handle response. renders /_error and set status code to res.statusCode if an error occurred.
  • app.renderToHTML(req, res, pathname, query) - similar to app.render but doesn't automatically handle response and returns a Promise of html string.
  • app.renderError(err, req, res, pathname, query) - similar to app.render but renders /_error (or the error page for debug if dev is true).
  • app.renderErrorToHTML(err, req, res, pathname, query) - similar to app.renderError but doesn't automatically handle response and returns a Promise of html string.
  • app.prepare() - wait to be prepared for serving.

Link

It takes a new prop as which is url value and href represents the path for the page component if as exists.

For example, it fetches the /foo component and url transitions to /bar if you click the following link.

<Link href="/foo" as="/bar">here</Link>

createServer((req, res) => {
const accept = accepts(req)
const type = accept.type(['json', 'html'])
if (type === 'json') {
Copy link
Contributor

@arunoda arunoda Nov 30, 2016

Choose a reason for hiding this comment

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

I didn't read the code in full, why do we need to do this?
If that's related to our .json file serving, can't app.render() could check the accept type and do something about it?

The idea is user has nothing to worry about our .json file serving?

Copy link
Contributor Author

@nkzawa nkzawa Dec 1, 2016

Choose a reason for hiding this comment

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

This is actually for .json file serving which is changed to use the Accept header with this PR.
The main reason we don't serve it using app.render is JSON endpoint for a component should be only one, so that you can use <Link href="/foo" as="/bar"> easier, for example.
Another reason is we have app.renderToHTML method too which you'd have to check the Accept header for using it anyway.
But I think it's arguable. Let me know if you have any ideas.

Copy link
Contributor

@arunoda arunoda Dec 1, 2016

Choose a reason for hiding this comment

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

Actually, my idea to remove this overhead from the user.

Anyway do we need to do this:

<Link href="/foo" as="/bar">

Since we are defining routes on the server. I think client doesn't need to know about anything about the actual mapping. So could do this:

<Link href="/blog/postId">

If we can somehow do this, this will be awesome.

Copy link
Contributor Author

@nkzawa nkzawa Dec 1, 2016

Choose a reason for hiding this comment

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

<Link href="/foo" as="/bar"> is for caching. A JSON response for a component is always identical, so we want to allow only <Link href="/blog" as="/blog/123"> instead of <Link href="/blog/123">

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. That makes sense.

Copy link
Contributor

@arunoda arunoda Dec 2, 2016

Choose a reason for hiding this comment

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

@nkzawa @rauchg how about on the other way.
Usually href means the URL display in the browser. So, how about something like this:

<Link href="/blog/123" for="/blog">

Copy link
Member

Choose a reason for hiding this comment

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

I like as for it implies decoration. We could also call it display?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also feel this is a bit confusing since href is an existing prop which we give a different meaning.

export default () => (
<ul>
<li><Link href='/b' as='/a'><a>a</a></Link></li>
<li><Link href='/a' as='/b'><a>b</a></Link></li>
Copy link
Contributor Author

@nkzawa nkzawa Dec 1, 2016

Choose a reason for hiding this comment

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

I think we should replace as with pathname, route or something like that, since you'd always like to write actual url to href.

Copy link
Contributor

Choose a reason for hiding this comment

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

I add some comments here about Link: #310 (comment)

I am not sure how to do this, but if we could find to use just <Link href='xxx'/> on the client that would be awesome.

app.prepare()
.then(() => {
createServer((req, res) => {
const accept = accepts(req)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means parsing the Accept header can happen twice, since we do it in our handler too :(

Copy link

@chiefjester chiefjester Dec 2, 2016

Choose a reason for hiding this comment

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

@nkzawa can't handle have an optional 3rd argument for parsing accept header, so that we just going to have to parse it once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. Alternatively, we can have two handlers, one is for JSON, the other is for HTML.

import HotReloader from './hot-reloader'
import { resolveFromList } from './resolve'

export default class Server {
constructor ({ dir = '.', dev = false, hotReload = false }) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hotReload option is removed since only dev: true, hotReload: true or dev: false, hotReload: false work for now anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this.
I don't think there's a much of a use case for dev: false and hotReload: true

We may need to enable hotReload in production for mobile apps, but we could do it later.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @arunoda. Very cool simplification

const app = next('.', { dev: true })
const handle = app.getRequestHandler()

app.prepare()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to wait app ready.

import Server from './'

module.exports = (dir, opts) => {
return new Server(dir, opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we can make dir optional and default to . (current directory).

Copy link
Member

Choose a reason for hiding this comment

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

@nkzawa please

app.render(req, res, '/b', query)
} else if (pathname === '/b') {
app.render(req, res, '/a', query)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to this basic switching, I think the example should also explicitly show how to make a parameterized route (e.g. /blog/:id). IMHO, this will mitigate tons of questions/confusion.

This is awesome btw!

Copy link
Member

Choose a reason for hiding this comment

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

+1 @jaredpalmer. We should have a basic example and a route pattern matching example

Copy link
Contributor

@jaredpalmer jaredpalmer Dec 2, 2016

Choose a reason for hiding this comment

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

@rauchg maybe i'm confused, but how do you get the params to the react page after matching on the server? this.props.url.params?

Copy link
Contributor

Choose a reason for hiding this comment

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

My temporary workaround is this:

let { pathname, query } = parse(req.url, true)

if (route('/blog/:id')(pathname).id) {
  query = query || {}
  query.params = route('/blog/:id')(pathname)
  app.render(req, res, '/blog__id', query)
} else {
  handle(req, res)
}

I feel like app.render should accept an optional params argument that gets passed into getInitialProps(ctx)?

Copy link
Member

Choose a reason for hiding this comment

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

I think that was our plan @nkzawa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that's the recommended way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the example of parameterized routing was added.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rauchg @nkzawa The Link implementation prevents access to the "desired" window object on the client of the decorated route during getInitialProps. You can test this by logging window.location in and out of getInitialProps and following a Link. The window.location will represent that of the href and not as, so things like are not accessible for data fetching. In contrast, the back button will give you the "correct" window.location during getInitialProps because it is a full render. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @nkzawa @rauchg ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaredpalmer you can use pathname and query arguments of getInitialProps instead of window.location.

@coveralls
Copy link

coveralls commented Dec 2, 2016

Coverage Status

Coverage increased (+11.0%) to 48.234% when pulling 849029d on add/programmatic-api into ea5ec12 on master.

@coveralls
Copy link

coveralls commented Dec 2, 2016

Coverage Status

Coverage increased (+11.0%) to 48.234% when pulling 302fe60 on add/programmatic-api into ea5ec12 on master.

@coveralls
Copy link

coveralls commented Dec 3, 2016

Coverage Status

Coverage increased (+11.0%) to 48.234% when pulling 147b647 on add/programmatic-api into ea5ec12 on master.

@coveralls
Copy link

coveralls commented Dec 3, 2016

Coverage Status

Coverage increased (+11.0%) to 48.234% when pulling d853586 on add/programmatic-api into ea5ec12 on master.

return
}

const { pathname, query } = parse(req.url, true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parsing url also can happen multiple times :(
Maybe handle should take pathname and query as optional arguments ?

handle(req, res, pathname, query)

Choose a reason for hiding this comment

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

+1 for this since its opt-in it wouldn't affect simplicity of the API.

Copy link
Member

Choose a reason for hiding this comment

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

@nkzawa or pass the un-destructured url object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

handle(req, res, pathname, query) seems better since it's same arguments with app.render().
But on handle(), we don't want to allow customizing pathname and query like app.render(), so un-destructured url object also makes sense, umm.

@coveralls
Copy link

coveralls commented Dec 6, 2016

Coverage Status

Coverage increased (+11.02%) to 48.229% when pulling 4fe3ba8 on add/programmatic-api into 7086287 on master.

@coveralls
Copy link

coveralls commented Dec 8, 2016

Coverage Status

Coverage increased (+11.02%) to 48.229% when pulling cc25662 on add/programmatic-api into 249a0c4 on master.

@knpwrs
Copy link

knpwrs commented Dec 12, 2016

#370 mentioned being able to use a database via a programmatic API. How would that work with this PR as it stands? My thought would have been some sort of way to pass data into the root react component or to define isServer and isClient variables via webpack that could then enable code trimming via uglify-js.

@queso
Copy link

queso commented Dec 12, 2016

Waiting on this to port a site to Next... What else is left here @nkzawa or @rauchg?

* Add custom-server-express example

* Remove extraneous nexts in express routes defs

* Update next config in server.js
mr47 added a commit to mr47/next.js that referenced this pull request Dec 15, 2016
@sarukuku
Copy link

I'd ❤️ to see this land soon.

* Handle accept headers totally inside Next.js
Now user doesn't need to handle it anymore.

* Move json pages serving to /_next/pages base path.

* Join paths correctly.
arunoda pushed a commit that referenced this pull request Dec 15, 2016
* fixed bug #362, also set chunks names

* set simple solution for minChunks

* revert in favor of #310
@@ -2,7 +2,7 @@
"name": "next",
"version": "1.2.3",
"description": "Minimalistic framework for server-rendered React applications",
"main": "./dist/lib/index.js",
"main": "./dist/server/next.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be incorrect. Looks like it should be ./dist/server/index.js

$ ls node_modules/next/dist/server      
build  config.js  hot-reloader.js  index.js  read.js  render.js  require.js  resolve.js  router.js

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind I see you haven't published a new version

const next = require('next')
const pathMatch = require('path-match')

const app = next({ dev: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on https://github.com/zeit/next.js/pull/310/files#r92954990, looking at ./dist/server/index.js the api seems to require new next({...}) to create the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh never mind I see you haven't published a new version

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
@ijjk
Copy link
Member

ijjk commented Mar 10, 2020

Failing test suites

test/integration/app-document/test/index.test.js

  • Document and App > Client side > should detect the changes to pages/_document.js and display it
Expand output

● Document and App › Client side › should detect the changes to pages/_document.js and display it

TIMED OUT: /Hello Document HMR/

Hello Document
Hi Document HMR
Hello App
Hello HMR
0.8709149041435691
index
about

  327 |     }
  328 |     console.error('TIMED OUT CHECK: ', { regex, content })
> 329 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content)
      |           ^
  330 |   }, 1000 * 30)
  331 |   while (!found) {
  332 |     try {

  at Timeout._onTimeout (lib/next-test-utils.js:329:11)

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.