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 Yeah I noticed the mess, but didn't want to make too many structural changes as it usually makes PRs harder to review. It's easier to do it separately and then run the tests to see who broke stuff :-)

[lukastaegert]

Hi @danielgindi! I started going through your PR. This looks really impressive!

It will probably take a moment until I am done. I hope you do not mind if I do some minor refactorings on your branch to get a better feeling for what is going on (as I wanted to do this anyway and your PR gives me an excuse 😜)?

[lukastaegert]

Also, it would be good to sync the fork with current master as there have been some smaller changes.

[danielgindi]

I don’t mind at all :) Ad we have a bunch of holidays here one after the other, I don’t have time to look into your comments and assist on the PR but I will get to it soon!

[danielgindi]

Wow you've done a lot of work there! I was just getting ready rolling up my sleeves but then I see all of that work. I see that you've done general refactoring to the library, things that I couldn't safely do without deeper knowledge of the library. Amazing!

[lukastaegert]

Thanks for the kind words. I must admit I really wanted this feature to go into the next release but at the moment it looks like there is quite a bit more to do. So my current plan is to extract the refactorings from the PR and but them into a separate PR so that I can focus on making the plugin Rollup@1.0 compatible (which was my original goal). Here is a list of what I did so far and what I believe should be done before this one can get merged:

• Renamed the option dynamicRequires to dynamicRequireTargets as I found it confusing initially whether it referred to the files being required or the files doing the requiring

• Added lots of test for the completely untested basic functionalities:

  • adding and removing targets with and without globs
  • requiring via relative paths
  • requiring with forward and backslashes in the path
  • requiring with and without extension works and returns the same instance
  • requiring json works if the json plugin is used (needed some fixing)
  • requiring directories via index.js/package.json works if the node-resolve plugin is present and we get the same instances if we require in different ways
  • requiring from node_modules folders works; needed fixing for node_modules on the same level

  With those tests in place, it should be a little easier to refactor code with some confidence. Nevertheless, there are probably quite a few untested things left. My rule of thumb is that removing or fundamentally changing a line of code should result in a red test. This also serves as the documentation and the insurance that refactorings do not break existing functionality. Most of the helper code is tested now but I did not look too deeply into the other code yet. Also, I could not figure out or write a test for the third if-statement case in commonjsRequire. There should either be a test or it should be refactored so that this case does not exist.

• the code left an untranspiled require statement for the path module. This had the result that created bundles were basically unusable in browsers or as ES modules. I solved this by adding a simple polyfill for the normalize function.
• under the assumption that internally, only forward slashes are used, some simplifications are possible. I did some of them but there are many more places where slashes are probably converted needlessly.

Things that need to or at least should be done before merging

• At the moment, the code assumes the first module read is the entry point, reads this module (ignoring other load hooks) and injects the dynamic module list. This is bad for several reasons:

  • It will not work with code-splitting
  • It will only work if no other load hook is required to read a file
  • It only works if the entry point is a CommonJS file because it always injects require statements (!!!!) This is really a no no as most Rollup projects will have ESM entry points and is a hard blocker against merging this PR

  It seems that the injection should probably happen in a transform hook instead and it should happen in all entry points (relying on the fact that rollup will dedupe the helper code).

• Local paths are coded into the module. I cannot imagine that many people would want this. I know you are aware of this but my feeling is there should be a way to fix this. One way of going about it could be to not handle absolute (i.e. starting with '/') require calls at all but forward them to an untranspiled require (as they will only work in node anyway). All other paths could then be resolved relative to a common local directory which could be the common ancestor of all dynamic require targets. node_modules resolution attempts could also stop at this common ancestor directory.
• The code does a additional IO even if no dynamic imports are taking place, making the plugin slower for everyone even if they do not use this option. This should be improved.
• The generated code looks somewhat inefficient.
  • The way imports of the new helpers are added always leaves a frozen namespace object in the generated code and retains more code than is necessary.
  • Each module is registered separately with the dynamic require loaders map while it should be possible to generate a pre-filled object that contains all the modules pre-wrapped, the same way as bundlers like Webpack and Parcel do it
• All new IO is sync which is causing further slowdowns. Except for plugin initialization, all used hooks can and should be async and return Promises so that rollup can handle other IO in parallel to make the build faster
• The third if-case in commonJsRequire should either be tested or removed

These are the most important points that struck me while working with the code. The main idea is very promising but personally, my target is to get this plugin to be rollup 1.0 compatible which I will focus on first. I will not be working on this PR for some time, if you would improve on some of the points listed above, this would be great!

[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