Implemented support for dynamic require

[danielgindi]

This can handle cases of dynamic requires (return require(modules[index]) / require(modulesPath + './test.js')) and circular dependencies.

Closes #131

[danielgindi]

Yay, finally passed all tests!
There's one weird thing with an additional \n that broke 2 tests, did not figure out why yet.

[ansarizafar]

@danielgindi Any update, when this feature will be released. I badly need this feature.

[danielgindi]

@ansarizafar It's not up to me...

I'll rebase on master as there are conflicts now

[ansarizafar]

@danielgindi will your implementation support complete dynamic path with require. Wepack does not support full dynamic path.
const location = '/project/modules;
const file = 'user,js'
example
require(location + file);

[danielgindi]

@ansarizafar Yes, but in case of fully dynamic path you'll have to tell the plugin to include those files so it can find it dynamically in runtime. You do that with the dynamicRequires option like in the example.
You can actually add the git url to the package.json instead of an npm version and see if it works for you.

[ansarizafar]

In my case path is fully dynamic and only known at runtime. It would be very useful If dynamicRequires option also allow a folder path from where to load files dynamically.

[danielgindi]

It does.

[danielgindi]

@guybedford are you going to consider merging this?
I've been working with this successfully for 2 months now with problematic modules like knex, logform etc.

[lukastaegert]

I will take look, probably early next week. This looks really promising.

From what I see from a first glance at the code: Maybe you want to split up some of the functions used. I know rollup-plugin-commonjs is some kind of a mess with this regard already so any works towards cleaning this up, at least as far as things are touched by this feature, is highly appreciated!

This will also make the review a little faster :)
Anyway, I am currently going over the core plugins to ensure rollup@1.0 compatibility, and having this feature would fit in nicely!

[danielgindi]

@lukastaegert As for the third if-case - that is resolved in my latest commits. I did some work on the path-module replicas there, as they had some bugs and broke a module that I'm compiling with this. Added tests for that too.

As for local paths hardcoded into the module - I'm aware of that, and in my own code which calls the rollup I'm replacing it with a root path. It could probably be done automatically, but not 100% as if someone has require calls to a module in a completely different path tree or a different drive - that would break things.

I'm not sure that we have to handle all complex cases right away with this. As this already covers most situations (that I stumbled upon, at least), and without this you have no other options anyway.
This also does not break compatibility, as it's optional - you specify dynamicRequireTargets, and without it nothing special is happening from this PR.

---

Btw there are many cases where dynamic requires are required that will be redundant in the future, as some modules are moving away from circular dependencies and dynamic requires. I posted quite a few issues in popular modules that do that, and most of them fixed it.
However some modules are more complex than others, like knex, where I fixed the issues with the module's core - but you still have to dynamically require the specific dialect that you are going to use.

[danielgindi]

Also what do you prefer - rebase on master or merge from master? As there are conflicts now

[lukastaegert]

Merging is ok as we will squash the PR anyway in the end

[danielgindi]

@lukastaegert I've added a proposal commit for handling the test with dynamic-require-es-entry, please review it!
It succeeds, but breaks form/unambiguous-with-default-export, form/unambiguous-with-import, and form/unambiguous-with-named-export.
Of course the output could be changed accordingly, but I want to make sure that this is the real expected output.

Also I see that in many tests the output contains imports from "commonjs-proxy:...". Is this really what's expected? And why don't those "proxies" pass again through the plugin? I don't see them passing in resolveId/load

[danielgindi]

@lukastaegert since there were so many conflicts with each merge due to the heavy refactoring - I've rebased this on master, and compared to the merged version to make sure that it's exactly the same.
This removed the heavy refactoring commits and let us stay with a steady branch

I need you to look at the tests - I think that the ones that are failing need to adjust their expected outputs. But can't be sure.

[lukastaegert]

Hi @danielgindi, sorry for the wait. The last weeks, I was busy with some important private matters as well as trying to finally get the rollup 1.0 release going, which for me has some priority as I hope you will understand.

It succeeds, but breaks form/unambiguous-with-default-export, form/unambiguous-with-import, and form/unambiguous-with-named-export

These tests make sure that a file that is already ESM (i.e. it contains import or export statements) is allowed to have a function named require without getting it transpiled. These tests should not be broken!

Also I see that in many tests the output contains imports from "commonjs-proxy:...". Is this really what's expected?

Yes, the form tests only test the transform function but not the entire process. These proxies are references to virtual modules injected by the plugin via the load hook. In the end, rollup itself resolves these imports via the virtual modules. The entire process is tested via the function tests (and the misc tests but the idea is to replace most of them with function tests if possible).

I don't see them passing in resolveId/load

But they do, it is the very first lines of resolveId (const isProxyModule = importee.startsWith(PROXY_PREFIX); if (isProxyModule) {...) as well as in the middle of the load function (if (id.startsWith(PROXY_PREFIX)) {...).

This removed the heavy refactoring commits and let us stay with a steady branch

This was my hope with cherry-picking them to a separate PR.

As for the future of this PR, this is why I am not happy to merge this in its current form:

• First and foremost, this PR is HUGE. Basically it adds about 50% more code just for this feature which is and should only be a niche use case as it produces very unoptimized code. Even without this PR, this plugin is in dire need of a large refactoring if not partial rewrite. I have some ideas how this might be substantially improved by adding a new function to the plugin context of rollup but adding all this new code will probably make it much less likely anyone wants to touch this plugin after this PR has been merged.
• This PR tries to do everything i.e. re-implement the whole node resolution algorithm not wondering if it actually makes sense to do so for a bundler. The greater problem here is that if we implement it all, then we have to support it all in the future (i.e. relative path resolution, absolute path resolution, package.json resolution and node_modules resolution), there is no easy way back. Also, this means users will have the complicated node_modules resolution algorithm in their generated bundle while I really doubt this is something anyone would want. So if anyone were to check if a file exists by importing it, it will iterate through all parent folders (which again depends on the system on which we bundled...) until it decides there is no hit. I would much rather have we split this PR and simplify the code as much as possible to ONLY support relative paths, probably including index.js and package.json resolution. Thus the algorithm can immediately bail if a path does not start with a ..
• The user file system dependent paths in the bundle are another big issue I have with this PR. These are bloating the bundle and leak personal information that we should not have in one of the most used plugins of the rollup ecosystem. If we restrict this PR to relative paths first, this offers a solution to the problem because instead of adding the absolute path of the importer to each import, we could determine a common parent path for all importing files and use a path relative to the common parent as an import base instead.
• Even though I added a lot of tests, there are still quite a few unstested corners in this PR. To just name a recent example, your refactored the normalize function in the helpers to handle ../.. paths correctly. So basically you fixed a bug in the code. Where is the test for that? Untested code cannot be refactored safely, especially not by a person that did not originally write the code (or by the person who wrote the code after a few months). As I said before, more than anything this plugin needs a thorough refactoring cleanup; also, this plugin is one of the most important plugins of the ecosystem. So please understand if I insist on good test coverage, especially for the helper code that is added to bundles.
• Lastly a personal reason: At the moment, most of all rollup PRs and issues go through me. And for me, the most important task at the moment is to finally get the 1.0 release going. So I do not really have enough capacities to work on this a lot and would rather have this PR delayed past the 1.0 release unless we can make it REALLY small.

So here is my suggested path forward:

• Remove absolute and node_modules path resolution. We can add that in another PR if necessary but please let's go small steps. Maybe even remove package.json resolution. This would MASSIVELY simplify everything and help us get the basics right before expanding upon them.
• Remove all logic related to that. Simplify the test helpers as much as possible. Bail if a path does not start with a . (e.g. by leaving the require untranspiled)
• TEST EVERYTHING! Check if commenting out blocks of code turn a test red etc.
• Make sure the existing tests pass again.

Sorry this is quite a long list of things to do. Note that even if we drastically reduce the scope of this PR, it does not mean the things we skip cannot be re-added at some later point. So make sure you keep a copy of the branch in its current state for reference.

[danielgindi]

Well a few points are already resolved AFAIK.

1. The extra code for the dynamic module resolution - is added to the bundle only if the rollup configuration specifies support for dynamic module resolution.
2. The issue with the normalize function is tested. I've added tests for that, to make sure that we're not missing these edge cases.
3. I don't see how removing steps from the actual resolution algorithm will benefit us. As it's a single function that follows node's resolving algorithm.
4. We could easily add flags to disable certain resolving features, for added security.
5. In one project I'm currently regex-replacing the outcome to remove user paths. We should be able to integrate it into the PR, but that will make it even bigger :-)

Overall the PR seems huge, but it's mainly tests...

I understand your priorities and considerations, it's OK if this takes a little bit more time.

[danielgindi]

@lukastaegert How can we get this moving?

[shellscape]

@danielgindi First, apologies that this didn't get the attention it needed. We've got a great team of developers that have joined in on a Plugin Maintainers team and we're migrating all of the rollup plugins to a monorepo at https://github.com/rollup/plugins.

I'd like to invite you to reopen your pull request on that repo once the move there is done. I fully recognize that it's a big ask. The unfortunate fact is that PR is just too large for us to get reviewed properly before we'd like to get the plugin moved. I also don't want to ask you to resolve any conflicts once again without a promise that it'll have eyes on it when it's done. I can tell you that a PR at the new plugins monorepo would have eyes on it.

Does that sound good to you?

[danielgindi]

@shellscape Yes, I'll try to get some time for it in the following days

[shellscape]

Thanks for replying, and no rush. I'll ping you again once we have the plugin moved to rollup/plugins.

[shellscape]

We've completed the move to the new repo. Please look for it in https://github.com/rollup/plugins