[RFC DRAFT WIP ...] refactor eval("require") workaround hack

[brodybits]

I noticed that the workaround solution of using eval("require")(...) is repeated throughout src/common/internal-plugins.js. I would really favor keeping this kind of a workaround hack in one place for DRY and to decrease the chance of missing places where we may want to improve this solution in the future.

This proposal is triggered by PR #7508, and I hope we can find a way to refactor instances of the workaround hack out of that PR as well.

I am raising this proposal as a DRAFT PR for now, and would be equally happy if this PR is closed in favor of a different solution. I really hope we can find a way to refactor this workaround solution somehow.

/cc @fisker

• I’ve added tests to confirm my change works.
• (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
• (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
• I’ve read the contributing guidelines.

Testing:

• I verified that npx eslint does not show any errors on the updated modules.
• I verified that npm test passes in my work area.

✨Try the playground for this PR✨

[fisker]

This is not how it works, that babel plugin only look for

eval('require')(module)
//              ^^^^^^ this is required

// and
eval('require').propOrMethod
//              ^^^^^^^^^^^^ this is required

[fisker]

If you want change that, you need rewrite scripts/build/babel-plugins/transform-custom-require.js, or maybe @rollup/plugin-replace can replace, not sure.

[fisker]

Or maybe try this

function requireModule(path) {
  return eval("require")(path);
}

[fisker]

For require.resolve we can do

requireModule.resolve = eval("require").resolve 

[brodybits]

I tried it, did not seem to get yarn prepare-release to pass.

[fisker]

It shouldn't

[fisker]

const requireModule = require; does this work?

[brodybits]

Seems to work, yes!

[brodybits]

I spoke too soon, prod build did not work.

If we do fix this on the prod build, we could remove our custom Babel transform-custom-require.js plugin and remove the useless require-module.js module that I added.

I think the main problem is this error:

[error] Dynamic requires are not currently supported by @rollup/plugin-commonjs

From some quick research I found an idea in winstonjs/logform#5 (comment) to use what is now @rollup/plugin-node-resolve (here on GitHub). Some very interesting reading in these sections:

• https://github.com/rollup/plugins/tree/master/packages/node-resolve#browser
• https://github.com/rollup/plugins/tree/master/packages/node-resolve#using-with-rollupplugin-commonjs
• https://github.com/rollup/plugins/tree/master/packages/node-resolve#resolving-built-ins-like-fs

I am now starting to wonder if we could do the whole prod build with an updated set of Rollup packages, with minimal or even zero Babel transforms, and minimal custom scripting.

Unfortunately I cannot promise when I will have extra time to look into this further. I would be happy if someone else wants to take this over.

[fisker]

Right, forgot that. rollup replace require to this error message.

[fisker]

As I said before. eval('require') actually a good one.

[brodybits]

eval('require') actually a good one.

Done but not working. I get this kind of an error on prod test:

Cannot find module '../language-html/parser-html' from 'index.js'

I tried hacking scripts/build/babel-plugins/transform-custom-require.js but with no luck.

The custom Babel plugin assumes that we are using eval('require') in every case where we need the transform, which is violated by this proposal.

I will now try another idea.

[fisker]

I mean eval('require') in the codebase is actually a good solution.

[fisker]

my code in fisker#302 is the way how to replace eval("require") to require, but if we do requireModule('./foo'), we can't locate the argument string now, so no way to replace that string

[fisker]

I think we shouldn't try anything more... All solutions I can think are less good than current solution .

[brodybits]

OK I am putting this on hold for now.

[fisker]

Don't try this way, even it works, we need hard code this function name.

[thorn0]

I unintentionally closed this and several other PRs because I deleted the next branch, but looks like a good solution wasn't found here and it's not really clear whether there is a problem to solve in the first place, so let's keep this PR closed.