PepsRyuu / nollup

Rollup compatible development bundler for fast rebuilds and HMR.
MIT License
488 stars 28 forks source link

Set an env variable when Nollup is running #50

Closed rixo closed 4 years ago

rixo commented 4 years ago

From Rollup's docs:

Note: While in watch mode, the ROLLUP_WATCH environment variable will be set to "true" by Rollup's command line interface and can be checked by plugins and other processes.

That would be great to have something similar with Nollup (e.g. NOLLUP or NOLLUP_WATCH).

It would allow to transparently adapt the behavior of a plugin without requiring the user to alter their Rollup config. Or to use workarounds like resolveId instead of resolve.

PepsRyuu commented 4 years ago

Apart from the workarounds, which ideally should be fixed (I want to try and implement the remaining APIs very soon), is there a specific plugins that come to mind? Does it make more sense to implement "ROLLUP_WATCH" if there's specific dev behaviour a plugin implements?

rixo commented 4 years ago

For the things that are gonna be fixed, having a sure mean to discriminate between Rollup and Nollup allows a plugin to implement the workaround internally while it's needed and remove it when it's not anymore, all while maintaining the same public API.

For example, autoCreate plugin sniffs Nollup to know if it's better to use resolveId or resolve (the plugin is useful because Rollup currently don't recover when you're importing a inexistant file). That saves the user from having to add an explicit nollup: true in the plugin config, and when the workaround becomes obsolete they won't have to change their config.

There are also differences that will never be fixed, because they're not even bugs.

For example, rollup-plugin-hot's API uses import.meta.hot while Nollup uses module.hot, plus other differences. rollup-plugin-hot is useless with Nollup (they do the same job), so it disables itself and throws in a compat layer when it detects Nollup.

rollup-plugin-svelte-hot uses the import.meta.hot API under the hood, so it sniffs Nollup to do the same thing, because I don't want to force people that only use Nollup to add rollup-plugin-hot to their config just for the compat layer.

The ideal outcome for the user is that Rollup and Nollup can consume the same Rollup config with the least amount of tweaking on the user's part.

The official Svelte's template uses ROLLUP_WATCH to discriminate between dev / production build. Why not? Rollup's about simplicity.

My Svelte hot template does the same thing to discriminate between Rollup and Nollup. But the catch in this case is that the env variable has to be manually set by the user, since it's not provided by Nollup. This is the very line where I want to scratch the NOLLUP=1. Because the users will want to write their own scripts in package.json, or even type the commands directly in the terminal. But some of them won't understand the env thing. Some of those who understand will forget. Often. They will get frustrated. They will consume support.

Fortunately, all this learning & teaching overhead can be avoided simply with a flag of sort that indicates whether we're being run by Rollup or Nollup (!ROLLUP_WATCH doesn't work because it can mean "not Nollup" or "not dev"). The flag just has to be public documented API, like Rollup, for it to be reliable. Then we can automate everything behind the scene, and these matters can remain a concern of only the plugin developers and the users that are actually interested in understanding. For the others, the vast majority I presume, it will just works reliably.

What do you think? Seems like a very good trade off to me!

PepsRyuu commented 4 years ago

Strong arguments, I like it!

From what I understand, it sounds necessary to have both process.env.ROLLUP_WATCH and process.env.NOLLUP both set to true. The first will deal with the Svelte template issue, so that you can easily plug and play Nollup which is one of the design goals for Nollup. The latter as you say is useful to determine if Nollup specific behaviours need to be added.

One concern of mine though is when these environment variables should be added. It's pretty clear that they should be added if you're using the CLI via nollup -c. However, unlike Rollup, there's also the dev middleware which provides watch functionality. Also, the Nollup bundler API might want to inject an environment variable so that plugins will behave correctly.

Here's what I propose:

ROLLUP_WATCH NOLLUP
nollup -c
Middleware
API

The only thing I don't like about the last two, is that they create side effects and pollute the environment. Not sure if that would be a concern. Would appreciate your thoughts on this proposal.

rixo commented 4 years ago

I was going to say "no", but actually you're right. If Nollup also emulates the ROLLUP_WATCH flag, then it can be transparent in a handful of additional cases. End-user facing cases. Good :)

In fact, I should be able to remove any mention of it in svelte-template-hot, if not for this one itchy occurrence. Same here, in good old test-case-hot-api, with rollup-plugin-serve. Any idea what I'm doing wrong? How can I configure separate output dir and serving dir for Nollup?

But sorry, I digress. Regarding your proposal, I'm all in favor of both variables from cli and middleware. For API... Err... Dunno. Do you have a pointer / example of which one is the "Nollup bundler API"?

But I suspect this might be unneeded anyway. If you just hack the value of process.env.NOLLUP before requiring rollup.config.js, everything will follow suit, since all plugins are required from it. No?

create side effects and pollute the environment

Oh? Which ones? OS? Shell environment? No... The node process? You mean changing process.env? Well, that's just part of the program's behaviour I'd say. Furthermore, Rollup does it, and we're playing double for Rollup, so we can blame them for any adverse effect... But personally, it feels like a pretty typical usage of environment variables. Is that what you were meaning?

However, strictly speaking it would be a breaking change. Because it is not inconceivable at all that someone using Nollup has already set their own NOLLUP env variable and given it another meaning (e.g. they've put a string in it for a path to something).

PepsRyuu commented 4 years ago

The examples cover the setup for managing output and serving directories.

output: {
    dir: 'dist',
    format: 'esm',
    entryFileNames: '[name].[hash].js'
}

As for serving content: --content-base public. This is the ideal setup as it gives you support for multiple different files.

There is a misalignment with Rollup for the file option it seems. When you call generate, the fileName is supposed to have the directory stripped. I've created a PR to correct this:

https://github.com/PepsRyuu/nollup/pull/53


The bundler API is this here: https://github.com/PepsRyuu/nollup/blob/master/lib/index.js#L503 Same thing as if using Rollup JS API. It's designed to be plug and play with Rollup. If I inject the NOLLUP environment variable into there, it will be applied everywhere.

When referring to pollution, I mean within the process. Ideally calling nollup() shouldn't have any weird side effects on the process itself, but perhaps I'm over-thinking it. I was thinking perhaps there's some strange use case out there that involves running Nollup, and then Rollup within the same Node process, one after the other. It would make the use case impossible unless you explicitly unset the env variable yourself. For ROLLUP_WATCH this isn't a problem, because it only exists in the CLI which is self contained and doesn't contain your own code. This is why it's a bit of a grey area for the middleware.

Perhaps the safest thing to do, is to only apply these environment variables when using the CLI only. Rollup doesn't add ROLLUP_WATCH when you call rollup.watch, it only adds it when using the CLI.

I'll be adding a check for the presence of the environment variable before setting it. This should prevent any issues if someone already has the environment variable set.

I'm leaning towards mostly this now:

ROLLUP_WATCH NOLLUP
nollup -c
Middleware
API
PepsRyuu commented 4 years ago

Environment variables added to CLI only as per Rollup. Released in 0.9.0.

rixo commented 4 years ago

Sorry, I'm a bit late on this one...

For the env variables, I think you're right, CLI only is the right decision. Otherwise it could just be confusing. What you've done completely covers my needs in this respects. Thanks.


When you call generate, the fileName is supposed to have the directory stripped.

Oh? Should it? That surprises me a little. Where have you seen that? I don't see it in Rollup's doc, is this reverse engineering?

That's a bit unfortunate, because I still have the output.file problem with 0.9.0 and the basename fix makes it harder to address... I'm opening another issue with the details.