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

Asset prefix support #2901

Closed
wants to merge 8 commits into from
Closed

Conversation

martwana
Copy link

@martwana martwana commented Sep 4, 2017

This PR extends the support for assetPrefixing - based off the comments on this issue: #257

This is dependant on a change to the webpack-hot-middleware component. PR here: webpack-contrib/webpack-hot-middleware#245

Usage:
To use this, we have to set the assetPrefix in our next config.
We also need to set there server to listen on that path and use next as the request handler. For Example, if your assetPrefix is '/metro', then a line like this is needed in your server config.

server.use('/metro', app.getRequestHandler()) // where app is next

@martwana
Copy link
Author

martwana commented Sep 4, 2017

Tests will fail due to the change to webpack-hot-middleware not being merged.

@@ -3,14 +3,20 @@
import Router from '../lib/router'
import fetch from 'unfetch'

const {
__NEXT_DATA__: {
assetPrefix

Choose a reason for hiding this comment

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

Should this be assetPrefix = '' to use empty string in case window.__NEXT_DATA__.assetPrefix is undefined?

@@ -52,6 +52,18 @@ export default () => {
}
}

const {
__NEXT_DATA__: {
assetPrefix

Choose a reason for hiding this comment

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

As above comment, a default value might help?

Copy link
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

👍

@martwana
Copy link
Author

martwana commented Sep 6, 2017

@nkzawa @arunoda Hey, do you have any idea how quickly you think we could get this merged?

@timneutkens
Copy link
Member

Reviewing this right now ❤️

@timneutkens timneutkens self-requested a review September 7, 2017 17:54
@timneutkens
Copy link
Member

Added the example for using the new feature 👌

Copy link
Member

@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.

This works really well. I've added an example showing how to use it. @arunoda we can merge this now 👌

Copy link
Contributor

@arunoda arunoda left a comment

Choose a reason for hiding this comment

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

I like to know about what'll happen to the Router.push API and <Link />.

Do we need to handle it automatically or ask the user to do it.

We use assetPrefix to add CDN support. With the ^^^ comment, I think we need to introduce a new flag called basePath or something.

If the assetPrefix is defined, basePath will be overridden for assets.

@@ -1,4 +1,4 @@
import webpackHotMiddlewareClient from 'webpack-hot-middleware/client?overlay=false&reload=true&path=/_next/webpack-hmr'
import webpackHotMiddlewareClient from 'webpack-hot-middleware/client?autoConnect=false'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the overlay option needs use here.
See:
screen shot 2017-09-10 at 3 47 12 am

(Try to add a syntax error)

@timneutkens
Copy link
Member

timneutkens commented Sep 10, 2017

I like providing the basepath.

Do we need to handle it automatically or ask the user to do it.

In this case I guess it'd be expected that we do it. Similar to how react-router does it.

@levino
Copy link

levino commented Oct 13, 2017

Is this still being worked on?

@timneutkens
Copy link
Member

@levino we're going to be actively working on this for next v5 👍

@levino
Copy link

levino commented Oct 13, 2017

Okay cool. Next.js is not really microservice ready without this. Will try out the beta version when helpful.

@sharif9876
Copy link

@levino why not? What's your use case?

@levino
Copy link

levino commented Oct 13, 2017

@sharif9876

Lets say I build an app that has a chatroom and a profile view. So it will be something like:

Url User story technology repo docker image
http://app.levino.io/chatroom A user can chat with other users next.js github.com/levino/chatroom hub.docker.com/levino/chatroom
http://app.levino.io/about A user can see terms of use static markup github.com/levino/about na
http://app.levino.io/profile A user can change their password next.js github.com/levino/profilePage hub.docker.com/levino/profile

As far as I understand microservices, I should be able to basically hit a different microservice with a different code base running in a different container, potentially on a different host with each path. So I can compose an app that feels like one consistent backend monolith in lots of small codebases that can be deployed independently. In the example above maybe the chatroom has errors and crashes, but then the profile and the about page are still accessible.

Edit: Added docker image paths to make the example clearer that both next.js apps run in individual containers in prod.

@podrivo
Copy link

podrivo commented Oct 24, 2017

Do we have a workaround for this at the moment?

@sharif9876
Copy link

sharif9876 commented Oct 24, 2017

@levino @podrivo Right now I don't think it's possible to host 2 next.js apps under the same domain (without using subdomains).

The main issue seems to be the /_next/ files that prevent this from working.

You can move your pages by putting them all in a directory. (e.g: App1 pages in 'pages/chatroom', App2 in 'pages/profile').

You can do something similar with your assets, and then putting an nginx infront to route '/static/chatroom' to the relevant app's static directory.

But we can't distinguish between /_next/ requests intended for each App. If there was a way to have it so that the /_next/ files could be served as /_next/chatroom/ then that would allow us to have a workaround (which is what is being worked on for next v5 I believe)

@brunopontes-hotmart
Copy link

@timneutkens Does your example work, Tim? I cloned this branch in order to do some tests but it's throwing 404 for _next folder files.

@timneutkens
Copy link
Member

@bapontes make sure you build and link next. The example is based on core changes in this PR.

@hnq90
Copy link

hnq90 commented Nov 13, 2017

IMHO, it's better to add basePath config instead of assetPrefix.

@levino
Copy link

levino commented Nov 13, 2017

@hnq90 I agree. But even better would be enabling the feature soon... Anyone looking at this?

@hnq90
Copy link

hnq90 commented Nov 16, 2017

@martwana Are you still working on this PR? If not, could I continue to take it?

@martwana
Copy link
Author

@hnq90 I stopped developing this when using basePath was suggested. It was more involved to change it to support that, and I'm not too familiar with JavaScript. We are currently running a forked version of Next.js from this branch in our applications.

The last I heard, this was going to be completed in Next v5. You're more than welcome to complete the work on it, but I'd suggest consulting with @timneutkens and @arunoda and the rest of the Next team, incase they have already done the work.

@hnq90
Copy link

hnq90 commented Nov 17, 2017

@timneutkens Hi, as @martwana mentioned above, is this feature already implemented? Or should I continue his work?

@timneutkens
Copy link
Member

@hnq90 we're going to work on it for V5. We have a few ideas as to how to implement it 👍 Arunoda is going to work on it 👌

@timneutkens
Copy link
Member

Going to close this for the implementation Arunoda is working on 👍

@levino
Copy link

levino commented Nov 23, 2017

Can you link the branch of that work?

@arunoda
Copy link
Contributor

arunoda commented Nov 23, 2017

@levino it's work-in-progress and not ready yet.
But we'll have something interesting pretty soon.

@tnulc
Copy link

tnulc commented May 20, 2018

Asset prefixing doesn't work as expected for me. I have an example repo here. Run yarn dev, and everything works as expected. Run yarn dev:prefix and inspect the network tab, and this is the result:

404 on next assets

I don't know if I'm misunderstanding how this is supposed to work, but it's stopping me from being able to deploy an app under a sub-path.

Looking at: https://github.com/zeit/next.js/blob/f2261050a0bc67e470dd18fa3fb70c507175012e/server/index.js#L192

Should all the urls in here not be prefixed with the asset prefix? I can patch it locally to make it work as expected.

@damusnet
Copy link
Contributor

damusnet commented May 21, 2018

@kirisu Are network requests failing in your example because you are accessing http://localhost:3000 instead of http://localhost:3000/prefix? My understanding is that assetPrefix will only add the prefix to javascript assets loaded by next. It doesn't do anything to the dev server, the router/links, or static ressources such as images you could be loading in your app.

In order to have your local dev environment behave like the production environment - in a subfolder - here is an example server script / proxy you can use:

const express = require('express');
const proxy = require('http-proxy-middleware');
const next = require('next');

const assetPrefix = process.env.ASSET_PREFIX || "";
const proxyPort = parseInt(process.env.PROXY_PORT, 10) || 9000;
const nextPort = parseInt(process.env.NEXT_PORT, 10) || 3000;
const dev = process.env.NODE_ENV !== 'production';
const app = next({ dev });
const handle = app.getRequestHandler();

const proxyServer = express();

proxyServer.use(
  '/',
  proxy({
    target: `http://localhost:${nextPort}`,
    pathRewrite: {
      `^/${assetPrefix}`: '/',
      '^/': '/404'
    }
  })
);

proxyServer.listen(proxyPort, proxyError => {
  if (proxyError) throw proxyError;

  app.prepare().then(() => {
    const nextServer = express();

    app.setAssetPrefix(`http://localhost:${proxyPort}/${assetPrefix}`);

    nextServer.get('/*', (req, res) => handle(req, res));

    nextServer.listen(nextPort, nextError => {
      if (nextError) throw nextError;
      console.log(`> Ready on http://localhost:${proxyPort}/${assetPrefix}`);
    });
  });
});

Run it with "dev": "NODE_ENV=development node server.dev.js", instead of "dev": "NODE_ENV=development next", in your package.json

It remains up to you to update a href and img src links to include the prefix.

@timneutkens
Copy link
Member

Or use github.com/zeit/micro-proxy

@damusnet
Copy link
Contributor

Hi @timneutkens

Actually, I tried that first, and for some reason I could never get it to work. Can you tell me what I'm doing wrong?

// rules.json
{
  "rules": [
    {
      "pathname": "/prefix",
      "method": ["GET", "POST", "OPTIONS"],
      "dest": "http://localhost:3000"
    },
    { "pathname": "/**", "dest": "http://localhost:3000/404" }
  ]
}
// next.config.js
module.exports = {
  assetPrefix: 'http://localhost:9000/prefix',
  exportPathMap: () => ({
    '/prefix': { page: '/index' }
  })
};
// package.json
{
  "scripts": {
    "dev": "micro-proxy -r rules.json -p 9000 & next"
  }
}

http://localhost:9000/prefix returns 200, but http://localhost:9000/prefix/_next/-/page/index.js is a 404, as is http://localhost:9000/concierge/static/images/icon.svg and other assets.

🤔

@joshuaquek
Copy link

joshuaquek commented Dec 4, 2018

I used:

In my routes.js file:

// ---- API Routes ----
const express = require('express')
const router = express.Router()

// -------- NextJs API Routes --------
router.get(`*`,  (req, res, next) => {
  return app.getRequestHandler()  // where app is nextjs app object
})

module.exports = router

In my server.js file:

const server = require('express')()
let routes = require('routes.js')
server.use('/my_custom_base_url', routes)

server.listen(port, (error) => { // Start server to listen on specified port
  if (error) throw error
  console.log('Server started.')  
})

and in my next.config.js file:

module.exports = {
  assetPrefix: '/my_custom_base_url'
  publicRuntimeConfig: { 
    staticFolder: `/my_custom_base_url/static`
  },
  webpack: (config) => {
    // Fixes npm packages that depend on `fs` module
    config.node = {
      fs: 'empty'
    }
    return config
  }
}

This fixed all of my routing issues and will make the base url of the entire nextjs webapp to have a base url of '/my_custom_base_url'

My version of NextJs is next@7.0.2

Hope this helps!

@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2019
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.