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

[plugin-transform-modules-commonjs] Does not always reference imports properly #10609

Open
dongryphon opened this issue Oct 28, 2019 · 4 comments
Labels

Comments

@dongryphon
Copy link

@dongryphon dongryphon commented Oct 28, 2019

Bug Report

Current Behavior
In some cases import does not generate correct reference to imported names.

Input Code

import {
    assert,
    Empty,
    chain, clone, copy, defineProp, is, merge, prototype, defProp
} from './Misc';

//...
@prototype({ /* ... */ })
class Config {
    // ...
    static mergers = {
        false (newValue, oldValue, target, mixinMeta) {
            return mixinMeta ? oldValue : newValue;
        },

        blend (newValue, oldValue, target, mixinMeta) {
            let key, ret;

            if (oldValue && newValue && newValue.constructor === Object &&
                oldValue.constructor === Object) {
                ret = clone(oldValue);

                if (mixinMeta) {
                    for (key in newValue) {
                        if (!(key in ret)) {
                            ret[key] = clone(newValue[key]);
                        }
                    }
                }
                else {
                    merge(ret, newValue);   // <<== BAD
                }
            }
            else {
                ret = (mixinMeta && oldValue != null) ? oldValue : newValue;
            }

            return ret;
        },
    };

The above reference to merge is left unchanged and hence does not refer to the imported merge function.

Expected behavior/code

All uses of imported names should be updated.

Babel Configuration

./babel.config.js

// eslint-disable-next-line no-undef
module.exports = function (api) {
    // eslint-disable-next-line no-undef
    const ES6_MODULES = process.env.ES6_MODULES === "true";

    api.cache(false);

    return {
        ignore: ['node_modules'],
        test: ['./packages/**'],
        plugins: [
            [
                '@babel/plugin-proposal-decorators',
                {
                    legacy: false,
                    decoratorsBeforeExport: false
                }
            ],
            [
                '@babel/plugin-proposal-class-properties',
                {
                    loose: false
                }
            ],
            !ES6_MODULES ? ['@babel/plugin-transform-modules-commonjs'] : null
        ].filter(Boolean)
    };
};

./babel.register.js

require('@babel/register')({
    ignore: ['node_modules'],
    test: ['./packages/**'],
    rootMode: 'upward'
});

Environment

  • Babel version(s): 7.6
  • Node/npm version: Node 12.8.1, npm 6.10.2
  • OS: Windows 10
  • Monorepo: Lerna
  • How you are using Babel: register

Possible Solution

I noticed the issue when I upgraded to 7.4. This works:

"devDependencies": {
    "@babel/cli": "7.1.x",
    "@babel/core": "7.1.x",
    "@babel/plugin-proposal-class-properties": "7.1.x",
    "@babel/plugin-proposal-decorators": "7.1.x",
    "@babel/plugin-transform-modules-commonjs": "7.1.x",
    "@babel/register": "7.0.x",

Additional context/Screenshots
image

In 7.1 the actual code looks like:

    (0, _Misc.merge)(ret, newValue);

In 7.6, it looks like this:

    merge(ret, newValue);
@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Oct 28, 2019

Hey @dongryphon! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@dongryphon

This comment has been minimized.

Copy link
Author

@dongryphon dongryphon commented Oct 28, 2019

I wish I could give you access to the repository but it is private.

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Oct 28, 2019

I could not reproduce this error on my local environment. Could you try remove the node_modules and reinstall?

@dongryphon

This comment has been minimized.

Copy link
Author

@dongryphon dongryphon commented Nov 2, 2019

I got the same result. Which wasn't surprising because this bug is like a light switch when I toggle the package dependencies between 7.1 and latest and then redo the lerna bootstrap. Works in 7.1 and fails in latest.

How can I help you debug this? If you can point me at how to debug the transform maybe I can spot something. I've never hacked on the babel before so I have no clue how to step through its processing. This bug is also totally consistent as to which import references do not get properly transformed so I don't think it will be hard to iterate on testing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.