-
Notifications
You must be signed in to change notification settings - Fork 26.2k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add flow, pages-manifest.json, defaultPathMap for export (minor) #4066
Add flow, pages-manifest.json, defaultPathMap for export (minor) #4066
Conversation
next export
to be ran without exportPathMap
(minor)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great with the functionality.
But we do some little improvements.
.flowconfig
Outdated
@@ -0,0 +1,3 @@ | |||
[ignore] | |||
<PROJECT_ROOT>/examples/.* | |||
<PROJECT_ROOT>/examples/.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why two instances of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
import { MATCH_ROUTE_NAME } from '../../utils' | ||
import {PAGES_MANIFEST} from '../../../lib/constants' | ||
|
||
export default class PagesManifestPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add a comment on what this plugin is about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
'> "next export" uses that function to build html pages.' | ||
) | ||
console.log('> No "exportPathMap" found in "next.config.js". Generating map from "./pages"') | ||
nextConfig.exportPathMap = async (defaultMap) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log a message on this is exporting all the pages or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generating map from "./pages"
seems clear to me 馃 @rauchg wrote the specific text update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the Generating map from "./pages".
It's bit of unclear what's map is. If the user created the exportPathMap
before it's clear.
Otherwise, he/she might be looking at what's this map
is.
@@ -1,5 +1,5 @@ | |||
import {join, parse, normalize, sep} from 'path' | |||
import fs from 'mz/fs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove mz
with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's still used, but there's another PR to remove it.
pages-manifest.json
that holds a mapping ofpage => js-file
this is now used for resolving pages inrequire.js
and to generate thedefaultPathMap
fornext export
. It also gives a major performance boost (again) because we don't have to hit the filesystem for non-existent pages 馃憣defaultPathMap
based onpages-manifest.json
next export
to be ran withoutexportPathMap