Dynamic require

[danielgindi]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

rollup/rollup-plugin-commonjs#131
rollup/rollup-plugin-commonjs#331

This has already been through major refactoring and adjustments, and now has been rebased on this repo.
Json support has been fixed (and this will now required the json plugin when requiring a full module dynamically, as it needs to cache the package.json).
Also the issue where the whole dynamic loader code was always added to any cjs project been rolled up is solved, it's now conditional on whether dynamic module loading is enabled or not.

Tests:
There are 3 tests (unambiguous-with-default-export, unambiguous-with-import, unambiguous-with-named-export) which were slightly modified.
The reasoning is that prior to this PR, if a file was detected to be using static import/export, then it was not processed for CJS require statements.
That situation was a nice optimization but not realistic, as users coming from webpack are used to have a JS file with static import/export, while importing or require a module which is essentially a CJS module. Such a module could have circular dependencies (i.e logform) or dynamic dependencies (i.e knex) which needs processing in the CJS realm.
Now these 3 tests are using require, which means that there might be surprises hidden behind the scenes, so it must be processed for CJS. This means that the output code is a little more verbose (though not ending up adding the CJS boilerplate).

[shellscape]

Thanks for reopening this PR here. Please fix the CI errors and we'll take a look once that's working.

[danielgindi]

@shellscape Are the node-vxx-latest tests being done with latest version over PR tests? Some weird combination?

[shellscape]

Not sure I understand the question. Pretty standard setup: linting runs on the files in the PR and all tests for the plugin(s) changed run.

[danielgindi]

I'm asking because the tests on Node 8/10 windows - pass.
Locally they pass on a mac with node 12.
Analysis in the CI - passes.
But node-vxx-latest tests do no pass, and they actually act like the new tests are running with the old code (which won't pass).

[danielgindi]

Found a potential cause for this. In the two tests that fail intermittently there was usage of rollup-plugin-node-resolve, which is not installed. I'm guessing it's installed somewhere on my local machine, and on one of the CI images, as sometimes it fails and sometimes does not.

[shellscape]

Well you should be leveraging @rollup/plugin-node-resolve (the old one is deprecated) and you should make sure it's added properly to the package's devDeps. Monorepos work a little differently. If we don't have this outlined in the README or CONTRIBUTING let me know and we'll add it to the docs.

[shellscape]

Thanks for working to get this passing CI.

One big thing missing from the docs is a code example that might require using dynamicRequireTargets. We get a lot of users from webpack and a lot of new users that are new to bundling and having a small code snippet in the docs to show what kind of code might require using the new option would be extremely helpful to users.

That aside I have several code quality comments I'd like to see fixed up along with some questions to be addressed.

@lukastaegert we'll need your eyes on this before we can merge it.

[danielgindi]

@shellscape Did some cleaning. I think it's ready as it already went through a lot of evolution (with the help of @lukastaegert), and it's a major part of what people are expecting when rolling up common js modules.
Also it's getting hard to rebase every time that the whole project changes (prettier, ava, repo change...).

[shellscape]

Also it's getting hard to rebase every time that the whole project changes

There's only been one major change so far, and that was for the migration to the monorepo. You shouldn't see any tooling changes for a long while.

We'll need resolution to #206 (comment) before we can move forward as well. I'm a stickler for making sure big changes are properly documented.

An aside: We've both ESLint and Prettier setup in the repo. I'd very highly recommend a plugin for your editor of choice which will automatically correct formatting/style on document save. It takes a lot of headache out of complying with linting guidelines.

[danielgindi]

Went over the notes, cleaned up a bit, and added info to the PR.

[shellscape]

Thanks. I'll bug @lukastaegert to take another look.

[lukastaegert]

Ok, this one is huge and I cannot promise when I will find time to look into this as my current stack of things to do is really long. One thing, I did not see any form tests that would allow me to inspect what the actually generated code is in the PR. Except deleted ones. Would it be possible to add some? I would not be too happy if the PR produced noticeably less efficient code for situations where we do not have dynamic requires.

[danielgindi]

@lukastaegert So are you suggesting restoring the 3 deleted ones and updating the expected outputs?

[lukastaegert]

That would definitely help with the review and might also prevent regressions in code quality. Why were they deleted in the first place?

[lukastaegert]

Alternatively, I think it would be even more interesting to see what Rollup makes of it in the end. There are already some function tests that do that via getCodeFromBundle, e.g. "produces optimized code when importing esm without a default export". But maybe we can do this as well for sample based tests. Let me think.

[lukastaegert]

Ok, here is a really simple change that will require little work of you but make the test much more powerful (and might even help us get rid of the explicit form tests eventually): Change test/function.js and add t.snapshot(code); as the last assertion for each test (i.e. after if (config.global) config.global(global, t);). Then each function test will also generate a snapshot that we can compare. I think this will also help with future reviews to see what changed.

[danielgindi]

That would definitely help with the review and might also prevent regressions in code quality. Why were they deleted in the first place?

Outlined in the PR description

[danielgindi]

I am not that (or at all) familiar with ava so I prefer not to change the tests in a way that I do not fully understand.

Anyway I found the root of the issue, brought back the tests with only a minor modification, and I'm updating the PR description.

[danielgindi]

@shellscape Any news on this?

[shellscape]

@danielgindi Apologies that I did not get to merging this before a conflict happened. Please run the following commands in the project to resolve that conflict:

git checkout master
git reset --hard origin && git pull
git checkout dynamic_require
git merge master
rm pnpm-lock.yaml
pnpm install --force
git add pnpm-lock.yaml
git commit --no-edit
git push

And you should be good. Every now and then the pnpm lockfile gets out of sorts and that's the fix for a conflict.

[danielgindi]

@danielgindi Apologies that I did not get to merging this before a conflict happened. Please run the following commands in the project to resolve that conflict:

Done!

[danielgindi]

@shellscape Don’t you want to merge it before breaking it again? 😂

[shellscape]

Gah. Done. Our day care closed so things are a bit bananas around here now.

Thank you very much for keeping steady on this one!

[danielgindi]

Gah. Done. Our day care closed so things are a bit bananas around here now.

Ours too, two weeks ago. It’s double bananas here. The girls are climbing on me all day, can’t do any serious work.

[justin808]

Any link to how to use the master branch to use this plugin?

Or any estimate on the publishing of the next version?

[mesqueeb]

I just got this error when trying to use a library of mine in which I use dynamic require:

Error: Dynamic requires are not currently supported by @rollup/plugin-commonjs

Should this have already been resolved by this thread? I am on

"@rollup/plugin-node-resolve": "^6.1.0",

[danielgindi]

@mesqueeb Please read the README. You need to use the latest version, and configure the dynamicRequireTargets option.

[mesqueeb]

@danielgindi Thank you very much!!!

[mjmnagy]

@mesqueeb Please read the README. You need to use the latest version, and configure the dynamicRequireTargets option.

Ive been trying to get this going per your comment and have had no success?

if i have a require statement like to:
this.svg = require('@icn/${set}/${icon}.svg')

would i need to list each possibility for set and icon?
ex:
@icn/brands/facebook.svg
@icn/light/lemon.svg
@icn/thick/lemon.svg

Thanks

[danielgindi]

@mjmnagy require('@icn/${set}/${icon}.svg') is a static require, not a dynamic one. But require(`@icn/${set}/${icon}.svg`) is dynamic.
Anyway yes you should pass a glob pattern that matches all modules that you want to be available for the dynamic require.

[mjmnagy]

@mjmnagy require('@icn/${set}/${icon}.svg') is a static require, not a dynamic one. But require(`@icn/${set}/${icon}.svg`) is dynamic.
Anyway yes you should pass a glob pattern that matches all modules that you want to be available for the dynamic require.

Sorry for the confusion - it was a translation error. The require is dynamic and I have provided the dynamicRequireTargets with a listing of files paths. Where should they be referenced from node_modules?

The function I have created to loop through the icons folders and create an array of strings of paths is::

const fs = require('fs')
const path = require('path')

const getAllFiles = function(dirPath, arrayOfFiles) {
  let dir = path.join(__dirname, 'dist', '/')
  let files = fs.readdirSync(dirPath)
  arrayOfFiles = arrayOfFiles || []

  files.forEach(function(file) {
    if (fs.statSync(dirPath + '/' + file).isDirectory()) {
      arrayOfFiles = getAllFiles(dirPath + '/' + file, arrayOfFiles)
    } else {
      arrayOfFiles.push(
        path
          .join(/*'node_modules', 'icons', */ '/', 'dist', dirPath, '/', file) //? not sure where the path should be from
          .replace(dir, '')
      )
    }
  })

  return arrayOfFiles
}

export default getAllFiles

No luck with this approach as I get Dynamic requires are not supported...
Any ideas would be greatly appreciated

[danielgindi]

dynamicRequireTargets: [
  'node_modules/**/*.svg'
]

[danielgindi]

Please just read the docs, also look at issues or open an issue. This PR is tested, merged, and working in production.

[danielgindi]

@lukastaegert please lock the conversation...

[mjmnagy]

Please just read the docs, also look at issues or open an issue. This PR is tested, merged, and working in production.

I have and I cannot get

dynamicRequireTargets: [
  'node_modules/**/*.svg'
]

to work hence why I'm asking