-
-
Notifications
You must be signed in to change notification settings - Fork 591
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
feat(commonjs): Add support for circular dependencies #658
Conversation
6c54d43
to
ec20e09
Compare
@lukastaegert It's awesome that you're working on this. We're in the process of considering wether or not to transition our build pipeline to another tool (because we hit this issue and working around it is non-trivial). I mean this as no pressure whatsoever, but if you have a sense of when you imagine this work will be complete and released, it'd give us a better idea of whether or not it's worth switching right now (I'd rather not). If you have no idea, that's fine too. Thank you! |
The branch is actually already in a working shape, but of course it is somewhat tricky to install from a PR. If you would try it out, though (e.g. like this https://stackoverflow.com/a/39195334), and see if this solves your issues, then this would be very helpful. Currently, I am working on reintroducing optimizations that I removed from the PR so that the generated code is not noticeably more complex than the code generated by the previous version. I expect to finish within the next week, but review and release will of course take longer. |
@lukastaegert I tried it out with zod's CJS build and it appears to be rather broken. Here's the result: https://gist.github.com/aaronjensen/15b4c47ab119b0b73d8d493c4574435d As far as I can tell, most of the code didn't actually get included. When it runs, it fails with:
Though that's not likely relevant given that most of the code is missing. |
Thanks for the feedback, though it is hard to tell form the gist what is missing because I would need to know how it was created.
I can only see a single file? So two things:
|
There are two, here are direct links: https://gist.github.com/aaronjensen/15b4c47ab119b0b73d8d493c4574435d#file-zod-circular-dependency-branch-js As for a repro, I set up: https://github.com/aaronjensen/rollup-zod-repro There's no rollup config there as I'm using whatever Snowpack uses natively. My guess is it's only the commonjs plugin that's mattering for this, but I'm not familiar enough with how Snowpack uses rollup to know for sure. |
I updated the repo with a rollup config and repro. |
Thanks a lot, I will check this out and use it to further refine this feature if the necessary changes are within the scope of the PR. |
ec20e09
to
7c7b4e4
Compare
The repo is perfect and will be really helpful, thanks a lot again! |
Ok, while I did not fix everything, I figured out the root issues and it looks really promising. First you did not see any code because due to the way the modules are now written, Without the sideEffects flag, it still fails, but this is now not a circular dependency issue but a simple interop issue (akin to: "stuff is on foo.default instead of foo"). I will figure out where it comes from and I am very confident this can be fixed as well. |
Ok, I was too confident about the interop issue. Unfortunately to fix that, we would also need to implement "perfect" CJS execution order which is just out of scope for this PR. To give you some context of what is happening: In the original code, you have something like this // foo.js
exports.foo = void 0;
const logFoo = require('./bar.js).logFoo;
exports.foo = 42;
logFoo(); // should log 42
// bar.js
var z = __importStar(require('./foo.js');
exports.logFoo = () => console.log(z.foo); Now when running this in Node what __importStar does is to detect the Now as Rollup does not yet support this properly, what happens instead is that in Rollup, In the meantime I wonder as your code looks very much like it was authored in TypeScript with ES modules if it isn't an option for you to actually run TypeScript with |
Hi @LukasHechenberger thank you for the additional details. I'm curious why the property needs to be defined. It seems that the only necessary thing is that the object reference is stable. That is, the object returned from I'm sure I'm missing plenty in terms of nuance here, so pardon my naiveté. As for the library itself, I'm unfortunately not the maintainer of |
Yes, that what happens on the master branch, and this is fixed in the PR branch, i.e. in the PR, the very same object is passed around. The problem is that |
Right, but would something like this work? var __importStar = (commonjsGlobal && commonjsGlobal.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) result = Object.create(mod);
__setModuleDefault(result, mod);
return result;
}; |
If it the helper would look that way, it would likely work for the current use-case. Problem is that the helper is part of each file. Of course you could provide a different version as a global variable, but this could likely mess up other people's builds. So it would need to be changed by a code transform e.g. via a plugin. But if this is a viable approach for you, why not? I actually gave it a shot with this REALLY simple version and it worked for zod, even making the output slightly smaller:
|
I guess my question is why not do something like I proposed for all files? What would that break? |
36c3ef4
to
ce489a1
Compare
So even though quite a few people tried this PR, there were no regressions to be found. I know there is still a lot going on here, can I do anything to get this moving? |
Would it be possible to add instructions on how to test this PR in the PR description (first message). I was not even sure if the PR would work with latest rollup nor how to test it. I'd love to give it a try if I have more instructions. Thanks for working on this ! |
See #658 (comment), but I will add this to the description |
@lukastaegert thanks for the link - I was not able to find it in the middle of the other comments. It works well on my main project. Unfortunately it doesn't completely solve protobufjs/protobuf.js#1503 - what I mean by completely is that the error is not the same - it seems like one circular dependency is fixed but there are still some issues. So I still think this is an improvement and should not block merging this PR. The only way to solve the issue might be to update protobufjs.
Repro:
My project https://github.com/vicb/flyxc could be used as a repro case. You only need to keep the second entry in you need to You can see the workaround here: Without setting
|
@lukastaegert if and only if you have a spare moment in the next few days, please drop a small status update on this PR. |
Oh, sorry. I kind of missed this one as I thought it was already merged weeks ago! I will updated it, this one should be released. |
Ok, working through the merge conflicts will take quite a while |
@shellscape Done. From my side, this should be released soon as this one quickly generates tricky merge conflicts. |
@lukastaegert will do. I want to get the dependency concerns taken care of before we release. Will do my best to get to that late today. |
- v18 added the `ignoreDynamicRequires` option as a breaking change. The earlier behaviour matched `true`, so I set it as the default (ref: rollup/plugins#819) - v19 is a breaking change since it now requires Rollup 2.38.0, but we’re already on 2.38.5. (ref: rollup/plugins#658)
I am requiring it like so and my rollup looks like this. I have no idea what's causing the break... export default {
input: 'app/client-entry.js',
preserveEntrySignatures: hot ? 'strict' : false,
preserveModules: hot,
plugins: [
svelte({
dev: process.env.mode === 'development',
preprocess,
hydratable: true
}),
alias,
hot && hmr({
public: 'public/dist',
serve: true,
inMemory: true,
host: null,
baseUrl: '/public/dist',
hotBundleSuffix: '',
}),
nodeResolve({
extensions: ['.js', '.svelte'],
browser: true
}),
json(),
commonjs(),
clientManifestPlugin(),
],
output: {
dir: distDir,
format: 'iife',
entryFileNames: hot ? undefined : '[name]-[hash].js'
}
} All of this was working in 18.1 |
- v18 added the `ignoreDynamicRequires` option as a breaking change. The earlier behaviour matched `true`, so I set it as the default (ref: rollup/plugins#819) - v19 is a breaking change since it now requires Rollup 2.38.0, but we’re already on 2.38.5. (ref: rollup/plugins#658)
Very appreciated. I managed to fix the error by using @rollup/plugin-commonjs@beta, even though I see the following warning, the compiled code works now.
|
Glad to hear! That warning has mostly informational quality (and there was lengthy discussion in the past if we want to keep it, but it has time and again proven helpful to people as circular dependencies can cause all sorts of issues). You can silence it with a custom |
Rollup Plugin Name:
commonjs
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
Now requires at least Rollup 2.38.0, otherwise the output may not work when
moduleSideEffects
are turned off.List any relevant issue numbers:
Description
To help with regression testing, I published a pre-release version as @rollup/plugin-commonjs@beta
This plugin brings quite a bunch of improvements, circular dependency support being the most important one.
Circular dependency support
This PR finally adds general support for circular dependencies to the commonjs plugin. Here is a contrived example that works in Node but does not work in the plugin at the moment:
So how does it work? When
main
importsdep
, it executes the file until it gets to wheremain
is required. Asmain
has not completed running,dep
will receive the current value ofmodule.exports
inmain
which is currently an empty object. Then it attachesgetMain
to its own exports.Back in
main
when we calldep.getMain()
, we receive this very object, which has now been enriched by the variablefoo
.In the previous version of this plugin, this code would fail because for ES modules, execution happens differently and there is no
module.exports
object that exists before the module is being executed.This will be fixed by this PR in the following way. First, a new helper module
/dep.js?commonjs-exports
with the following content is created:This is then referenced in the transformed version of dep.js
The main point here is that we import
__exports
from/dep.js?commonjs-exports
instead of creating it locally, then attach our exports and reexport it. Due to how Rollup works, this will cause the binding fordep
to be created much earlier so it can be referenced in all modules of the circular dependency. It also matches how real commonjs modules behave.This also required a change in Rollup to work when
moduleSideEffects
are turned off by the user: Now when a module imports a variable from a modulebar
that is imported itself inbar
from a modulefoo
and then exported again inbar
using a regular export (not a reexport), then all side-effects inbar
are guaranteed to be included if the variable itself is included.This is what the generated output now looks like:
Quite a bit nicer. Note how the
dep
andmain
object are created at the top now to be referenced by all subsequent code.No automatic wrapping for nested assignments to exports or module.exports
Previously, assigning exports in nested conditions or functions forced a module to be wrapped in a function. This is no longer the case. E.g.
As you can see, the output is very much optimized. If we were to import an ESM named export from
dep.js
, however, the output code would look different:Beautiful, isn't it? So how does it work? The actually transformed module looks like this:
So it uses these double assignments
foo = dep.foo = 'foo'
whenever the export is reassigned. Recent improvements in Rollup now allow us to partially tree-shake these assignments, depending whether eitherdep
orfoo
is referenced in the end. If both are referenced, both are retained.Support for circular dependencies even when there is a nested assignment to module exports
As stated before, even nested assignments to module.exports do not force the code to be wrapped any more. This is achieved by instead of
/dep.js?commonjs-exports
, we now injected a different helper module/dep.js?commonjs-module
that looks like this:Additionally, this module has
__module
configured as a synthetic fallback namespace for non-existing named exports. The effect is that when I importexports
from this module, I actually referencedep.exports
, and this reference behaves as a live-binding, tracking reassignments. Here is an example, what this looks like (without a circular reference, though):Special handling for top-level module.exports reassignments
So far, we have seen two helper modules used to support circular references,
/dep.js?commonjs-exports
and/dep.js?commonjs-module
. Now when there is an unconditional assignment to module.exports in the file, there is no reason to keep those around. After all, everything will be replaced. In such a situation, Rollup optimizes the code by turning those assignments into variable declarations:This even works for multiple assignments as
var
declarations can be re-declared.Optimized wrapping
Now there are still situations where we want to wrap code in a function for improved spec compliance. Mainly when
module
orexports
is reassigned, this is the best way to handle it. But also when there are references to bothexports
andmodule.exports
in the same file, it is hard for the logic to get it right. Now instead of the old logic that produced something like this:doing a double function wrapping (i.e. the module is wrapped in a function that is passed as a callback to yet another function to be called, with the corresponding runtime overhead), the plugin will now generate an optimized IIFE with a single function call:
Again you can see we access the
/dep.js?commonjs-module
helper module, so wrapped code supports circular references as well! But there is also yet another level of optimization: If module is not accessed at all, then instead again the code uses/dep.js?commonjs-exports
and also optimizes the wrapper:Now isn't that nice?
Now there is one disclaimer: While this PR adds support for circular references, it does not add support for exact execution order for inline require statements. As some packages with circular references also rely on that, they may still not work. But often, they can now be made to work by manually tweaking the execution order via strategic imports, which is a big improvement already.
So, I now need people to try this out. Code-level review is also extremely welcome, but I understand it may be overwhelming. Trying this out on large code-bases would be really helpful to find regressions as well. I can at least state it works for Rollup, and it also worked for the apollo-server starter example.