Create full path to distDir

[dizlexik]

I tried setting distDir to dist/client and my build failed since dist didn't exist yet. This should resolve that issue.

Btw, I didn't have time to do a proper pull request for this so I did a quick file edit directly on GitHub. Seems safe enough for this minor change.

[rauchg]

+1, but a test would be nice

[dizlexik]

Agreed. I'll try to get to it later tonight if I get a free moment.

[dizlexik]

So I've been looking into this more deeply and I think there was some dangerous code introduced in #1599 that needs to be fixed ASAP.

For example, in my project I have src/client/next.config.js where I thought it might be nice to add distDir: '../../dist/client'. I looked through the Next.js code and I see buildDir = join(tmpdir(), uuid.v4()) and then later _buildDir = join(buildDir, dist), which is scary because now fs activity is happening outside of the confines of the temporary build directory, not good.

I don't see any good reason for the changes to server/build/index.js in #1599. It seems to me that distDir is completely irrelevant to the build process other than for the very last step within server/build/replace.js that moves the results from the temporary build directory to their final location.

I really wish I could take this on myself but I just don't have the time right now. But I do think this needs some serious attention as soon as someone can. Particularly in both server/build/index.js and server/build/replace.js.

As for my minor modifications in this pull request, I think they can be disregarded for now and once these other issues are cleared up this can be readdressed. It should be as easy as adding something like await mkdirp(join(_dir, '..')) before await move(_buildDir, _dir) in server/build/replace.js.

[alexnewmannn]

@dizlexik If i remember correctly @dizlexik the only changes in index and replace were passing in the location where the build takes place in order to get that config, the join with those buildDir folders have already been there, the only additional was LOCATING the config so that it can create it in the correct place. Because again, if I remember correctly, without those changes it was trying to locate files in .next that didn't exist because that's what it was expecting. The addition of the extra params in those functions is to literally get the config. I will try removing the changes in index and see if that is the case. but yeh those two join functions weren't added as part of #1599, they were just modified to remove the hardcoded .next param. Ill have a look at this in a few hours.

[alexnewmannn]

@dizlexik I have a fix for this, the 3 tasks work as normal, theres a little change made to the webpack config but build, start, dev all work fine and create the folder as intended. They just do it in different ways.

[rauchg]

Merged @alexnewmannn's PR

[alexnewmannn]

@rauchg not sure my PR fixes what this one was trying to achieve, was just fixing the additional work he was planning

[rauchg]

I see, so we still need to run mkdirp correct?

[alexnewmannn]

yes i believe so

…

On Fri, 7 Apr 2017 at 18:04, Guillermo Rauch ***@***.***> wrote: I see, so we still need to run mkdirp correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1645 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFExrygg81ZES61Ais5-1mNp8VIC5llyks5rtmy1gaJpZM4M2ERq> .

[dizlexik]

Thanks @alexnewmannn for fixing that distDir issue.

I made this mkdirp change again to the latest master but now I'm running into another issue related to distDir that's preventing me from running in production mode. Within the constructor in server/index.js I see this.config = getConfig(this.dir) where this.dir is my dist directory which contains no config file so the default config is used. Immediately following that is this.dist = this.config.distDir so this.dir ends up equaling the default distDir value of .next. Then after that is this.buildStats = !dev ? require(join(this.dir, this.dist, 'build-stats.json')) : null which of course throws an error because there is no .next directory within my dist directory.

I don't know how many other areas may be affected by this, but I quickly skimmed through the rest of that file and I see at least a few other places where this.dist is being joined to this.dir and I suspect these will fail as well.

I'll hold off just a bit longer on my change until I'm able to produce a reliable production build.

[alexnewmannn]

i think i'm following, maybe this is highlighting we should have tests that cover a production build? i'm on my phone at the moment, are you available on the zeit slack? i'm also thinking we could potentially pass the config in here, although i need to read the code properly when i get to a computer https://github.com/zeit/next.js/blob/9347c8bdd010742c55dde14c7975afb8e8704cf2/bin/next-start#L51

[alexnewmannn]

Tried running this in similar circumstances to @dizlexik and can't recreate. Production builds run fine, also with koa as used by dizlexik. Will take a look at this extra issue if recreation steps are possible :)

[dizlexik]

I think I see where the disconnect is here. My build process also includes building my server-side code, which ends up in dist/server, which then loads the Next client code from dist/client. It seems that currently Next is unable to run properly when loaded directly from the dist directory. It instead requires the server to load it from its source directory because there is now a runtime dependency on the configuration file. I think this dependency was introduced with the new distDir change and needs to be eliminated. Does that make sense, @alexnewmannn?

[alexnewmannn]

I think so, can you maybe upload a zip of a hello world build process/project in a simplified form? If so i can take a look, otherwise ill just be guessing

[timneutkens]

I'm going to close this.