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

feat: transform in common js #47

Merged
merged 6 commits into from
Aug 11, 2024
Merged

Conversation

Shunjun
Copy link
Contributor

@Shunjun Shunjun commented Aug 4, 2024

if a commonjs module imported by es module.the global name will be transformed. but a commonjs module imported by another commonjs module it will not be transformed.

in this case if bar is global.

- node_modules:
  - bar:
    - index.js: | 
        module.exports = "BAR";
  - foo:
    - index.js: | 
        const bar = require("bar"); // <- this will not be replaced, and alse will be packaged
        console.log('foo');
        module.exports = (val) => console.log(val || bar);
- entry.js: |
    import log from "foo";
    import bar from "bar" // <- this will be replaced
    log(bar);

I think this is not right. so i transformed it after @rollup/plugin-commonjs.
And i added a new config to control this feature, default is closed.

@Shunjun Shunjun changed the title transform in common js feat: transform in common js Aug 4, 2024
@eight04
Copy link
Owner

eight04 commented Aug 7, 2024

It seems that commonjs transforms the absolute require bar into its full path path/to/bar/index.js and write it into the source (commonjs-proxy). Therefore external-globals doesn't know that full path is an external module.

I think the correct solution is: external-globals should register a resolveId handler which runs before commonjs. When finding an identifier matching the global variable map, mark it as external so commonjs won't try to expand the id into its full path.

external-globals should not transform commonjs modules.

@Shunjun
Copy link
Contributor Author

Shunjun commented Aug 8, 2024

I think you're right. I'll fix it later

Copy link
Owner

@eight04 eight04 left a comment

Choose a reason for hiding this comment

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

LGTM

package.json Outdated
"c8": "^10.1.2",
"endent": "^2.1.0",
"eslint": "^9.5.0",
"globals": "^15.8.0",
Copy link
Owner

Choose a reason for hiding this comment

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

What does globals do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because you imported it in eslint.config.mjs. transitive dependencies is not work in pnpm.
I've deleted it.

test/test.js Outdated
}]
},
{
transformInCommonJs: true
Copy link
Owner

Choose a reason for hiding this comment

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

We can remove this option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

test/test.js Outdated
}]
},
{
transformInCommonJs: true
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@Shunjun
Copy link
Contributor Author

Shunjun commented Aug 10, 2024

I found another case that behaviour is not as before.

Suppose "bar" is the external module.

// foo.js
export * from "bar"

// index.js
import {a} from "foo.js"

console.log(a);
  • Before
    Rollup will check the bar module, and write into index. Although external-globals didn't do anything with "export * ".

I think it's also problematic that 'a' in global bar maybe not as same as in local bar

// output
const a = 1;
console.log(a);
  • Now
    The bar has been external. so rollup doesn't check this module.
// output
import {a} from "bar"
console.log(a);

now the bar can not be resolved.

I'm not sure that's a problem, maybe user should not export * from an external global module
But to resolve this problem I think we need transform code again in renderChunk.

@eight04
Copy link
Owner

eight04 commented Aug 11, 2024

export * from "bar"

It's a known issue that external-globals doesn't work with export all syntax:
#25

transform code again in renderChunk

This will only work if output.format = "es".

@eight04 eight04 merged commit 443c690 into eight04:master Aug 11, 2024
1 check passed
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.

2 participants