Skip to content
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

WIP: Fallback to the process.cwd to search for modules #12986

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexander-schranz
Copy link

@alexander-schranz alexander-schranz commented Mar 9, 2021

Q                       A
Fixed Issues? babel/babel-loader#895
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? No (need some help)
Documentation PR Link
Any Dependency Changes?
License MIT

If you have some uncommen directory structure like: https://github.com/alexander-schranz/build-error-reproducer. Babel can not find the plugins or presets. See issue babel/babel-loader#895. A fallback to the process.cwd does fix it, not sure if this need is the correct place to fix this, also if other places need to keep in mind.

@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/44169/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 9, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 36eb2cf:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration


try {
return require.resolve(standardizedName, {
paths: [dirname],
paths: dirnames,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this doesn't work because we compile away this call for compatibility with older Node.js versions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that the additional parameter not just ignored for older node versions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require.resolve is tranpiled to an in-house polyfill:

babel/babel.config.js

Lines 260 to 269 in b3e2bcd

((v,w)=>(v=v.split("."),w=w.split("."),+v[0]>+w[0]||v[0]==w[0]&&+v[1]>=+w[1]))(process.versions.node, "8.9")
? require.resolve
: (/* request */ r, { paths: [/* base */ b] }, M = require("module")) => {
let /* filename */ f = M._findPath(r, M._nodeModulePaths(b).concat(b));
if (f) return f;
f = new Error(\`Cannot resolve module '\${r}'\`);
f.code = "MODULE_NOT_FOUND";
throw f;
}
`,
so we can support Node.js 6, it will be removed in Babel 8 though.

I think we can support general paths by applying M._nodeModulePaths(#).concat(#) to every element.

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently Babel presets/plugins are resolved against the dirname of the input file. In most cases when Babel and the app libraries are installed within the same root folder, this is equivalent to resolve from process.cwd(), but that's not the case when the Babel plugins are separated from app codes.

Note that although dirname defaults to process.cwd(), it is always the dirname of the input source when we create a plugin/preset descriptor from babel config

export function createUncachedDescriptors(

which is invoked by root(), env() ... in

function makeChainWalker<ArgT: { options: ValidatedOptions, dirname: string }>({

I guess this is also why CRA and many other dev tools use require.resolve or require instead, because Babel by default does not resolve from process.cwd().

Note that we can not remove defaults for dirname here as it is supposed to work with babel.createConfigItem API.

I think the PR is in a good direction. I would like to hear others opinion.

We also should document how Babel resolve plugins/presets on https://babeljs.io/docs/en/babel-core#configitem-type


try {
return require.resolve(standardizedName, {
paths: [dirname],
paths: dirnames,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require.resolve is tranpiled to an in-house polyfill:

babel/babel.config.js

Lines 260 to 269 in b3e2bcd

((v,w)=>(v=v.split("."),w=w.split("."),+v[0]>+w[0]||v[0]==w[0]&&+v[1]>=+w[1]))(process.versions.node, "8.9")
? require.resolve
: (/* request */ r, { paths: [/* base */ b] }, M = require("module")) => {
let /* filename */ f = M._findPath(r, M._nodeModulePaths(b).concat(b));
if (f) return f;
f = new Error(\`Cannot resolve module '\${r}'\`);
f.code = "MODULE_NOT_FOUND";
throw f;
}
`,
so we can support Node.js 6, it will be removed in Babel 8 though.

I think we can support general paths by applying M._nodeModulePaths(#).concat(#) to every element.

@nicolo-ribaudo
Copy link
Member

Note that there has already been a lot of discussion about supporting custom resolutions, but they have also been rejected since JS configs can already use require, require.resolve, or whatever resolution strategy they want: https://github.com/babel/babel/issues?q=sort%3Aupdated-desc+NODE_PATH+

@nicolo-ribaudo
Copy link
Member

@alexander-schranz I cloned your repository, but I don't understand what problem it tries to show.

The error you are seeing is not from https://github.com/alexander-schranz/build-error-reproducer/blob/2142a91af9d8ffafcf3c2a65fb9ed93277d33fc5/assets/admin/babel.config.json#L9. Babel is correctly able to resolve that @babel/plugin-proposal-decorators.

The error comes from https://github.com/alexander-schranz/build-error-reproducer/blob/2142a91af9d8ffafcf3c2a65fb9ed93277d33fc5/vendor/sulu/sulu/src/Sulu/Bundle/AdminBundle/Resources/js/.babelrc#L4, and that package clearly specifies @babel/plugin-proposal-decorators in its dependencies. You need to run npm install inside vendor/sulu/sulu, so that sulu's dependencies are installed.

@alexander-schranz
Copy link
Author

alexander-schranz commented Mar 9, 2021

@nicolo-ribaudo

and that package clearly specifies @babel/plugin-proposal-decorators in its dependencies. You need to run npm install inside vendor/sulu/sulu, so that sulu's dependencies are installed.

That should not be the case running in all this repository again npm install would end that peer dependencies like mobx and react don't work longer as expected as not longer the peer version is used, also we can not force all our users to run npm install in all there installed php packages so the vendor is a directory like node_modules you actually should do nothing there.

Maybe babel is the false repository to fix the issue as I node itself has the problem to resolve it correctly. Npm install all dependencies correctly also the dependencies of the linked packages like it should but node can not correctly resolve the path to them as it seems not take the process.cwd into account where the dependency actually was installed.

@alexander-schranz
Copy link
Author

@JLHwung

Note that we can not remove defaults for dirname here as it is supposed to work with babel.createConfigItem API.

Does adding a additional path change this? Possible that to implement this just as dirname where node should look for it. So as fallback? I think I'm missing the impact. Any place where we can hook in that it would only look in process.cwd before throwing the no module found error so its really just a fallback to the process.cwd directory and doesn't have any inpact to the other cases you listed?

@nicolo-ribaudo

Note that there has already been a lot of discussion about supporting custom resolutions, but they have also been rejected > since JS configs can already use require, require.resolve, or whatever resolution strategy they want: https://github.com/babel/babel/issues?q=sort%3Aupdated-desc+NODE_PATH+

Oh I see, I'm really not sure if the problem should maybe be fixed on the node side so I will put this currently on hold maybe I can get there some feedback.

Thx you both for the fast response, really appreciate this!

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 9, 2021

Tbh I don't think that this will/should be fixed on Node.js's side either. Babel plugins are like normal devDependencies (if your users shouldn't care about them) or dependencies (if your users need to care about them).

Since in your case "you" (vendor/sulu/sulu/src/Sulu/Bundle/PageBundle/Resources/js) are asking the "user" (assets/admin) to compile your code, your code should specify the Babel plugins it needs as dependencies.

However, when using npm's file: protocol, dependencies are not installed and npm expects you to manually run npm install (or npm install --only=prod) in every "vendored" dependency. You can verify this by replacing vendor/sulu/sulu/src/Sulu/Bundle/PageBundle/Resources/js/.babelrc with a .babelrc.js file with these contents:

console.log(require("classnames"));

module.exports = {
    "presets": ["@babel/preset-env", "@babel/preset-react"],
    "plugins": [
        ["@babel/plugin-proposal-decorators", {"legacy": true}],
        "@babel/plugin-proposal-object-rest-spread",
        "@babel/plugin-transform-flow-strip-types",
        ["@babel/plugin-proposal-class-properties", {"loose": true}]
    ]
}

classnames is a dependency of vendor/sulu/sulu/src/Sulu/Bundle/PageBundle/Resources/js, but Node.js cannot find it and it will throw Cannot find module 'classnames'.

@alexander-schranz
Copy link
Author

@nicolo-ribaudo we don't want that the babel plugins which are needed to compile to be added as dependencies in the bundles (submodules) package.json, so they are added also to the main package.json. If you do require("classnames") in the index.js you will see there its correctly resolved, the webpack resolver is configured to resolve this dependencies correctly. This is done over the following config in the webpack.config.js:

        resolve: {
            modules: ['node_modules', nodeModulesPath],
        },
        resolveLoader: {
            modules: ['node_modules', nodeModulesPath],
        },

It would be great if the webpack babel-loader could keep this config in mind. Do you know if that would be possible that we read this webpack resolve config and forward this folder as dirnames to the plugins resolveStandardizedName function. For BC something like:

function resolveStandardizedName(
  type: "plugin" | "preset",
  name: string,
  dirname: string = process.cwd(),
  resolverDirNames: array = []
) {
     const dirnames = [dirname, ...resolverDirNames];

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Mar 10, 2021

@alexander-schranz I now see why we didn't understand each other: Babel packages are a build-time dependency, while all the other dependencies sulu have are runtime (in the browser) dependencies.

I still think this should be solved not on the Babel or Node.js side but by reorganizing how sulu specifies its dependencies, but I have a few questions:

  • Is sulu always in a vendor folder, or it would usually be a normal dependency of assets/admin?
  • vendor/sulu/sulu specifies two dependencies: sulu-admin-bundle and sulu-page-bundle. However, you never install them. In the repository you provided you are lucky because the "user" (assets/admin) has the same two dependencies so Node.js can resolve them, but how do you make sure that it always happen?
  • vendor/sulu/sulu/src/Sulu/Bundle has different dependencies (such as classnames), but you never run npm install in this folder. How do you make sure that they are available?
  • Shuold the responsibility of ensuring a correct compilation be on assets/admin or vendor/sulu/sulu? In the first case you can just ignore sulu's .babelrc and define all the necessary plugins in assets/admin/babel.config.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants