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

Output dir, public dir, and URLs #84

Closed
rixo opened this issue May 18, 2020 · 34 comments
Closed

Output dir, public dir, and URLs #84

rixo opened this issue May 18, 2020 · 34 comments

Comments

@rixo
Copy link
Contributor

rixo commented May 18, 2020

So, this is a bit of a follow up to #56.

I am encountering issues with URLs in several projects I'm trying to Nollupize. I've identified 3 causes:

  • the directories in output.dir are discarded from URL, which defers from Rollup
  • entry / chunk files with same basename overwrite each others
  • in some cases, I need to have a separate dist and public folder, and I need URLs from Nollup to be adjusted accordingly

I've created a repo to illustrate and help diagnostic the issues: https://github.com/rixo/nollup-test-case-output-dir

The problems are described in detail in the readme.

I've gone fancy and used a test runner that is not yet very stable, especially with Windows... Let me know if it doesn't work for you, I'll try to simplify the setup.

@PepsRyuu
Copy link
Owner

Haven't tried running it yet, but will do so shortly, so this is just some initial thoughts which might help in the meantime!

The chunks naming collision is a good catch. Will need to sort that one out.

As for the other two issues, that appears to be working correctly based on the readme doc, when compared to Rollup's generate behaviour and Webpack's output.path. When you call generate(), the bundle metadata will say nothing about dist as that property is only relevant when writing to disk, same with Webpack. Webpack ignores output.path unless it is writing to disk. Nollup follows this convention as writing to disk shouldn't be a thing in development.

Have you tried using entryFileNames? You could set dir: "dist" and then set entryFileNames: "app/[name].[hash].js, and that will preserve app in the URL.

Will look into the demo more when I get a chance later this evening. :)

@rixo
Copy link
Contributor Author

rixo commented May 18, 2020

Yes, indeed, it seems to match with Rollup's bundle.generate, but with bundle.write, Rollup does preserve the directories. I'm totally with you that writing to disk during dev is best avoided, however running Nollup is more akin to running Rollup watch (or bundle.watch) + a web server than running bundle.generate. And Rollup's watch does call bundle.write, and so we've got (part of) the directories in the file URLs.

This is more a question for the web server side of Nollup than it's bundler side actually. I don't say it should change in the generated bundle (it shouldn't since that's what Rollup does), but the directories should be preserved in the URLs, like with Rollup + web server.

With entryFileNames and output.dir options, the file paths produced by Rollup and Nollup also differ:

=== output.dir + entryFileNames ================================================

{ contentBase: 'dist' }
{
  input: 'src/a/main.js',
  output: { dir: 'dist/app', entryFileNames: 'app/[name].js', format: 'es' }
}
Rollup:
 ⚠  'dist/app/app/main.js' => /app/app/main.js
Nollup:
[ 'app/main.js' ]

With Rollup, the file would be accessible at URL /app/app/main.js, while with Nollup at /app/main.js.

Anyway, relying on entryFileNames / chunkFileNames would be Nollup-specific. That would mean that, without those options, Rollup and Nollup don't behave the same. In particular, bringing Nollup into existing projects with output.dir would basically mean to have to tweak those options to make it work. Not a single chance of drop-it, it works, woohoo!

@PepsRyuu
Copy link
Owner

It's worth noting, Rollup watch has an option to not write to the disk: watch.skipWrite, so that Rollup watch doesn't call bundle.write(). The intent of which is so that it can be more easily integrated with a web server to serve bundles from.

There's a few problems here, and it's mostly down to interpretation and architecture. There's several ways to write a Rollup development server. Before writing Nollup, I used Rollup's generate function to generate a bundle in memory, and then to serve that bundle using Express. Rollup ignores dir for generate() and relies on entryFileNames to determine the path of each generated file. Because it was getting incredibly slow, I created Nollup to be a drop in replacement and it worked for that setup.

That's how I did it, but then someone else can use Rollup entirely differently. Someone might use the write API, and solely use entryFileNames to serve the content, or they might use a mixture of dir and entryFileNames to determine where to serve content from. Someone might copy build artifacts into a source code directory (public). Since generate() writes to memory by default, it's more aligned with Nollup's objective so the dev server uses that flow. Focusing on generate prevents some plugins from writing to the hard drive during development as well which is also another reason to focus on that flow instead of the write flow.

Another issue here is the interpretation of dir. In my opinion, dir should only be a path that contains content to be served, it's path shouldn't be part of the content. In other words, be the same as Webpack's output.path, which webpack-dev-server ignores entirely. If I had something like the following:

dir: "target/prod/web/esm"
format: "esm",
entryFileNames: "app/[name].[hash].js"

I would expect the files to be served as app/[name].[hash].js. I wouldn't expect prod/web/esm to be in the URL because that is intended to be just a path that describes the built artifacts inside. In other scenarios such as the one you've mentioned, it's impossible to determine intent here, and I think only removing the first part of the path would lead to some very obscure behaviour. I'd imagine there would be more complaints with requests to disable the feature, than problems solved.

If a developer is already using the Rollup API with a custom web server, the low-level API is more or less a drop-in replacement and you can generate quick dev bundles. The web-server though, as there's no precedent, it's more opinionated, but I think there's good reason to believe that basing it on generate() is the way to go. For projects that are hoping to replace their entire web server setup, I just don't see how it can be a drop-in replacement as there's so many different interpretations and flows.

I think the implementation that Nollup has at the moment is correct, and is the best balance for all that makes it easy to integrate with minimal changes, while also maintaining consistency with all other bundler development servers.

And so it's not forgotten about, chunking name-collision I'll be looking into! :D

@rixo
Copy link
Contributor Author

rixo commented May 18, 2020

I'm sorry but I can't get to agree with this. To me it seems that the dev server should be producing the same entry points with the same URLs as the "prod" server. Since rollup -cw writes and preserves directories, I believe nollup -cw should simulate writes and preserve directories, for easiest integration and predictability.

Considering only the generate workflow and discarding the output.dir from the URL opens a class of conflict problems of the same kind we have to deal for chunk names for entry points. Consider the following config:

export default [
  {
    input: 'app1/main.js',
    output: { dir: 'dist/app1', format: 'es' }
  },
  {
    input: 'app2/main.js',
    output: { dir: 'dist/app2', format: 'es' }
  }
]

By discarding the output.dir, Nollup will overwrite one of the bundle at URL /main.js. If I try to fix the situation with entryFileNames, then the URLs I must use in index.html won't be the same for Rollup and Nollup.

To me it still seems that the simplest behaviour to understand and use is for Nollup to pretend to write the same things as Rollup, and pretend to serve from a given fs path. With this we can derive all the same URLs for dev and prod, and if there are naming conflicts that would be because they exist in the prod setup as well.

That being said, there might be something that I don't get in this dev server approach... But the required change to support the ISO write workflow in the dev-middleware doesn't seem to be huge. Would you consider supporting this as a Nollup option (something like --virtual-write maybe)?

@PepsRyuu
Copy link
Owner

I'm okay with supporting an option for this behaviour, but the problem is that the behaviour is still very ambiguous as to how it should behave with dir. In the example I referred to:

input: './src/main.js',
output: {
    dir: 'target/prod/web/esm',
    format: 'esm',
    entryFileNames: 'app/[name].[hash].js'
}

What would be the result for when entryFileNames is present, and for when it is not present?

@rixo
Copy link
Contributor Author

rixo commented May 18, 2020

With entryFileNames, it would produce the "virtual" fs path target/prod/web/esm/app/main.[hash].js. With no entryFileNames, it would produce: target/prod/web/esm/main.js (same as rollup -c).

Then, the URL would be produced by trimming the contentBase, for example with contentBase: 'target/prod/web', we'd get URLs /esm/app/main.[hash].js and /esm/main.js, respectively.

An additional option would be needed (e.g. baseUrl or publicPath...) to handle the case where contentBase point to another directory (e.g. static), different from the dist directory, and both are merged for prod only. In this case, this additional option would be used instead of contentBase to convert fs paths to URLs.

@PepsRyuu
Copy link
Owner

I'm not sure reusing contentBase makes sense here. It's different from the previous file scenario as discussed a while back, as we are writing to a directory that already exists and that you're serving static content from so there was no issue. Here though, the directory doesn't exist, and there would probably be static files that you also want served.

A suggestion here, expanding upon --virtual-write, something like:

--virtual-write target/prod/web

This would say to follow a full write flow, but when serving content, serve from this directory. Would something like this work? This would allow contentBase to still point to public or any other static directory that exists.

@rixo
Copy link
Contributor Author

rixo commented May 18, 2020

Yes, I think it would work. The value of the --virtual-write option would play the role of the option I was mentioning earlier (as baseUrl or publicPath). Making it an option that accept a value is far better naming indeed.

Reusing contentBase would work for the simplest case where we're generating bundle files in the public directory. I think it should be the default when --virtual-write is just true, but I'm not hell bent on this one.

@PepsRyuu
Copy link
Owner

Another question here for --virtual-write is whether or not it should trigger the writeBundle lifecycle hook. At the moment this hook has not been implemented at all (since no writing occurs at any stage). Would the expectation be that this hook will be triggered in plugins? I'm worried about plugins that do actually write to disk and modify files and potentially causing errors. But at the same time, other plugins might want this.

@rixo
Copy link
Contributor Author

rixo commented May 18, 2020

I would say no.

Called only at the end of bundle.write() once all files have been written.

Plugins would expect the files to actually exist on FS and, indeed, that could go awry.

This hook not being called is part of the normal Rollup workflow, when using generate (!) so plugin are expected to support when this hook is never called.

From a theoretical point of view, I consider the --virtual-write option as a dev server option, as opposed to a bundler option. I think you're right that sticking to the generate workflow for the bundler is simpler for everyone (plugins included).

@PepsRyuu
Copy link
Owner

#88 Thoughts?

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jun 8, 2020

#85 (comment)

@rixo Continuing the discussion with it here.

Not sure I understand the issue with generating the manifests. Virtual write shouldn't affect any plugins. How are the manifests being created? Any reference links?

@rixo
Copy link
Contributor Author

rixo commented Jun 9, 2020

Regarding the discussion before Sapper, I'm not so sure it was a genius idea to drop files from the bundle if they fall outside of the "public" dir (got bitten by it already).

Actually I now don't think virtualWrite is the right approach :-x. The desired result can be achieved with a simpler baseUrl option, that would just get prepended to fileName to form the URL. With the added bonus that there are no unspecified cases...

Unfortunately, ideally this option should probably live in output of the Rollup's config because, in multi bundle configs, the base URL may differ per bundle (also true for virtualWrite, in fact). But hijacking Rollup's config with Nollup options doesn't seem genius either.

Maybe a map FS path => base URL (or URL => path?), for complex situations? Maybe we don't care for now?

Now, back to Sapper...

Not sure I understand the issue with generating the manifests. Virtual write shouldn't affect any plugins. How are the manifests being created? Any reference links?

In Routify and Svench, the manifests are created by an external process that watches a specific slice of the src directory (typically something like "pages") and writes a .js file (the "manifest"). Currently, they're both using absolute FS path in dynamic imports, in their manifest.

For Sapper, I don't know so much, but I believe it takes fileName from chunks in the bundle result generated by Rollup to generate its dynamic imports. I know it also uses info from the bundle result to know the URL of the entry point (see bellow).

For example in Sapper, you end up with some code like this that is fed to the bundler:

export const components = [
	{
		js: () => import("../../../routes/index.svelte"),
		css: "__SAPPER_CSS_PLACEHOLDER:index.svelte__"
	},
	{
		js: () => import("../../../routes/about.svelte"),
		css: "__SAPPER_CSS_PLACEHOLDER:about.svelte__"
	},
	{
		js: () => import("../../../routes/blog2/index.svelte"),
		css: "__SAPPER_CSS_PLACEHOLDER:blog2/index.svelte__"
	},
	...
]

Anyway, I think I have mischaracterized the problem in my previous comment. The problem really boils down to Nollup's __nollup__import__map__ + virtualWrite (or baseUrl, or mounting the Nollup middleware on a non root URL).

For example, in one of my setup, I have something like this:

rollup.config.js

  output: { dir: `public/build`, format: 'es' }

.nolluprc.js

  virtualWrite: 'public',

  // this is something else I'd like to discuss, someday...
  watch: ['src', '.svench/*', 'node_modules/svench/tmp'],

index.html

    <script type="module" src="/build/main.js"></script>

Now, let's say somewhere in my code I have an import like this:

import('./foo.js')

Then the dynamic import generated by Nollup will try to fetch the chunk at URL /foo-[hash].js, when it's really available at /build/foo-[hash].js... And that's the problem!

I've managed to fix it in Svench by patching utils.js like this, above this line (and using baseUrl: 'build' instead of virtualWrite):

// context.baseUrl also added by the patch
if (context.baseUrl && (entry.isEntry || entry.isDynamicEntry)) {
  entry.fileName = (context.baseUrl + '/' + entry.fileName)
}

But with Sapper it doesn't work. Guess why? Because it breaks alignment with Rollup, as you've always said!

Specifically, Sapper generates the entry script URL (in index.html) dynamically and, to do so, it uses fileName from the bundle result. It also prepends (hardcoded) client/, because it's where it's serving this bundle from. But with my patch, fileName is, for example, client/main-[hash].js (expected URL), and so I end up with index.html fetching client/client/main-[hash].js (incorrect!).

So... What do you think?

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jun 9, 2020

Web apps running in a URL subdirectory always seem to occur with mind-boggling headaches! D:

It sounds like the publicPath option in Webpack: https://webpack.js.org/guides/public-path/ But as you've implied, there's no such equivalent in Rollup. Googling Rollup and public path funnily enough lead me to this: surma/rollup-plugin-loadz0r#9

I'm very reluctant to start adding custom options to Rollup config as well. Because this involves the generated code, we have to look at this from the perspective of calling nollup() API directly. That means we have to use the Rollup config somehow.

Here's one thing that I did think of that might work, assume the following option existed in .nolluprc:

publicPath: '/client' // applies to all inputs
publicPath: ['/clientA', '/clientB'] // applies in order to all arrays of configs and all inputs

Like in Webpack, all assets and scripts will only be available if you have that URL prefixed, and dynamic imports will have it prefixed as well.

I have a couple of ideas on how I can get this to work, mainly by taking advantage of https://rollupjs.org/guide/en/#renderdynamicimport. I'm thinking the dev middleware could inject a custom plugin that uses that hook to inject the public path. The hook is not implemented yet in Nollup, and I'm not entirely sure how it would work with the dynamic import map but I think it's feasible.

What are your thoughts on that option in .nolluprc?

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jun 9, 2020

Also worth noting, Webpack doesn't have multiple public paths options (seems to be refusal to implement it and people are using workarounds). So there's no precedent here from what I can tell to take inspiration from.

@rixo
Copy link
Contributor Author

rixo commented Jun 9, 2020

Yes, this is Webpack's publicPath in the end... I still find the description in the docs confusing for such a simple concept, but I finally get it. Sorry for sidetracking the solution with my virtualWrite thing :-x

So, yup I think it would solve the reported problem. I'm all for it.

I'm ambivalent about the multi-value array form. I find it clunky but I can think of a better alternative. Postponing the multi value support until a brilliant idea or a real need would be ok with me.

I didn't know about this hook. Seems to have a lot of hack potential! If you can get a new hook supported in Nollup in the process, it's all the better.

So there's no precedent here from what I can tell to take inspiration from.

I think Snowpack's mount directive is very much related, even if not exactly the same thing.

Something similar could probably solve most cases where different publicPath for different output would be desired. Web servers routinely mount different directories / files on specific URLs. I guess that's what would be doing (and trying to replicate in Nollup) an app with this need. I don't have such a use case at hand though, so that remains theoretical for me.

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jun 9, 2020

Just occurred to me, using renderDynamicImport is probably a bad idea, because it stops the developer from using it for other plugins (I should implement it regardless though).

😑

I don't think there's any clean way of solving this. Probably will have to bite the bullet and pass a custom property in Rollup config.

Going to think about this one more...

@PepsRyuu
Copy link
Owner

Humour me, does the latest refactor branch work?

I've noticed that Rollup outputs relative URLS, and that imports resolve relative to the script that imported them, not to the URL, which means they don't need to start with / and will work perfectly fine inside an SPA using non-existing URLs.

I've pushed a new example example-public-path that demonstrates this feature in use.

@rixo
Copy link
Contributor Author

rixo commented Jun 10, 2020

Yes, it works in Svench. It's the easiest setup, that requires the less patches. Mainly, it's now working without any invasive patch (only surface things, will report...). It's still using dynamic imports & one setup needs publicPath. Seems to work perfectly (will do deeper testing, I'm actively working on this thing).

I'm trying to hook it into Routify & Sapper as soon as I can (probably tonight).

@rixo
Copy link
Contributor Author

rixo commented Jun 10, 2020

Interesting: looking into Routify now... It uses output.file with inlineDynamicImports option. I think it's gonna work for dynamic imports (a big stuck on something else, can't confirm yet), except that I was getting an absolute path in the files array of the middleware, only for the "bundle" file (i.e. output.file). Just commenting out the whole special treatment for output.file fixes it apparently. And with publicPath, I'm getting the bundle at the URL I want / expect.

@rixo
Copy link
Contributor Author

rixo commented Jun 10, 2020

Still have to diagnose the cause (or make a clean repro) but apparently, in Routify, Nollup has a hard time swallowing this named export:

export const {tree, routes} = buildClientTree(_tree)

I get {'undefined': undefined} instead of the routes export. Apparently tree and the others exports are fine.

If I rewrite the above line as follow, I do get the expected result:

const result = buildClientTree(_tree)
export const tree = result.tree
export const routes = result.routes

@PepsRyuu
Copy link
Owner

Never knew that was even possible. Can you write that into a separate issue?

Need more details on the files issue.

@rixo
Copy link
Contributor Author

rixo commented Jun 10, 2020

Plugged into Sapper with publicPath. Seems to work. I haven't seen the issue myself in Sapper, but it was apparently the same I had in my other project, so I'm pretty confident it should be fixed here too. I'm going to share this in the Sapper issue to get more feedback.

I'm creating repros for the other issues I've mentioned here.

@rixo
Copy link
Contributor Author

rixo commented Jun 10, 2020

Like I've mentioned in the other issue, I've got Nollup to work with Routify.

I didn't need the output.file bundle actually for dev / HMR, just the ESM build.

I wasn't able to reproduce this file issue in a more controlled environment but, anyway, I think it really boils down to inlineDynamicImports not being implemented by Nollup. All the tests I've made with just output.file and no dynamic imports were ok.

inlineDynamicImports is not a current need for me. Do you think it's worth opening an issue now? It seems that implementing this option would involve quite a bit of code moving, wouldn't it?

@PepsRyuu
Copy link
Owner

inlineDynamicImports would definitely create some complications. But for record keeping, would be worth creating an issue, although I don't think it will be in the refactor since it seems like a limited use case that can be easily worked around.

@rixo
Copy link
Contributor Author

rixo commented Jun 10, 2020

I would personally prefer for it not to be included in the current refactor because it would increase the risk of introducing new bugs all while making it harder to find their root cause.

@PepsRyuu
Copy link
Owner

I can see the problem with the file option but I'm not sure what the solution is. It always uses the full URL, unless it starts with contentBase and strips that. If we only output the filename, that will break subdirectory use cases.

Perhaps the contentBase rule can be removed in favour of virtual write? Seems like it solves the issue.

@rixo
Copy link
Contributor Author

rixo commented Jun 11, 2020

Yes, if I remember correctly, the contentBase patching was intended to solve the same thing as publicPath. Apart from breaking change consideration, I think only publicPath should remain. I believe it can solve all the cases (if it supported publicPath per output, but same for contentBase).

@PepsRyuu
Copy link
Owner

So here's a few scenarios to consider:

Scenario 1:

index.html  -- <script src="/dist/main.js">
rollup.config.js -- file: "dist/main.js"

Scenario 2:

index.html -- <script src="/main.js">
rollup.config.js -- file: "dist/main.js"

Scenario 3:

public/index.html -- <script src="/main.js">
rollup.config.js -- file: public/main.js

Scenario 4:

public/index.html -- <script src="/build/main.js">
rollup.config.js -- file: public/build/main.js

Will need to figure out a proposal that works for all of these and more potentially...

@PepsRyuu
Copy link
Owner

Just thinking about this further, and I'm not sure if there's any right answer at all for output.file. It's just far too ambiguous to determine what is intended to be a compilation directory, and what's intended to be part of the URL.

The ability to use Nollup out of the box with the "recommended" template of outputting to public directory is a very compelling argument and use case. I think this functionality is important to keep, and will avoid breaking people.

I'm torn on the "what if it doesn't start with contentBase" situation though. I can see both arguments being made for both keeping and omitting the directory from the URL, and it's not like workarounds don't exist either (entryFileNames with dir). Also considering there's quite a bit of usage now for Nollup since the decision was originally made, I'd be a bit worried about breakage for this particular case.

So I think the refactor will proceed as is (assuming there's no other issues).

@rixo
Copy link
Contributor Author

rixo commented Jun 15, 2020

Makes sense.

Regarding your previous message, I think publicPath can cover all the cases. It's a bit less automatic than the idea of virtualDir in some cases, but it also doesn't come with a big hole in the middle so, well...

publicPath is still included in the refactor, right? In this case, maybe it could just take precedence over the output.file special case? If publicPath is not used, current behaviour would be preserved.

The refactor seems good to me. I've had another bug in another pretty exotic rename in chain situation (very corner case, not very important) but, apart from that, everything seems to be humming good. I've got friends at Routify starting to use it on their own too, and I've not been reported any issue for now. Seems solid!

@PepsRyuu
Copy link
Owner

publicPath will be released in the refactor.

I'm not sure if I want to tie publicPath to the behaviour of output.file as it still leaves it far too ambiguous and makes a complex and messy option even more messy. Not to mention, you still might want to prepend publicPath to the entirety of output.file.

Only other thing I can think of is to have an obscure option similar to --virtual-write like --output-file-dir which will remove from output.file the starting directory.

file: 'dist/lib/bundle.js' ---> /dist/lib/bundle.js

// --output-file-dir=dist
file: 'dist/lib/bundle.js' ---> /lib/bundle.js

That's great news! Once it's been tested a bit further, I'll tidy it up for a full release!

@PepsRyuu
Copy link
Owner

FYI, going to tidy up the Refactor branch in regards to documentation, and aim to have a full release this week. :)

@PepsRyuu
Copy link
Owner

Released in 0.12.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants