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

fix: CJS export default #673

Merged
merged 5 commits into from
Nov 11, 2024
Merged

fix: CJS export default #673

merged 5 commits into from
Nov 11, 2024

Conversation

richarddavison
Copy link
Contributor

@richarddavison richarddavison commented Nov 11, 2024

Description of changes

CJS imports via ESM now returns the default export if present. This should help with #668 and maintain better consistency with Node.
@nabetti1720 even tho all testes are passing, can you verify this doesn't change expected CJS import behavior from ESM?

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@nabetti1720
Copy link
Contributor

nabetti1720 commented Nov 11, 2024

@nabetti1720 even tho all testes are passing, can you verify this doesn't change expected CJS import behavior from ESM?

@richarddavison , I built commit#c8a60db on my laptop and checked the require calls for the packages I have tried so far and no regressions seem to occur in them.

However, in reproduction tests of previously resolved issues, regressions seem to be occurring.
#641 (comment)

NOTE: llrt-clone built commit#c8a60db.

  1. There is no change in behavior when importing js files:
// hello.js
var hello = function world() {
  return "hello world"
}
module.exports = hello;
// main.js
import hello from './hello.js'
console.log(hello())
% llrt main.js      
SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/hello.js'

% llrt-clone main.js
SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/hello.js'
  1. Regression occurs when importing cjs file:
// main.js
import hello from './hello.cjs' // The content is exactly the same as hello.js
console.log(hello())
% llrt main.js      
hello world

% llrt-clone main.js
SyntaxError: variable name expected
  at /Users/shinya/Workspaces/llrt-test/hello.cjs:1:111

@nabetti1720
Copy link
Contributor

I have not been able to confirm the assert module that was uncovered this time because it has not been merged into the mainline. If you could rebase it after the merge, I can check it for you. :)

@nabetti1720
Copy link
Contributor

The following error still occurs when importing, but the situation has not changed since before this fix, so it is a different issue. I would like to verify more after this PR is merged...

SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/node_modules/lodash.merge/./index.js'
  
SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/node_modules/bowser/es5.js'

@richarddavison
Copy link
Contributor Author

The following error still occurs when importing, but the situation has not changed since before this fix, so it is a different issue. I would like to verify more after this PR is merged...

SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/node_modules/lodash.merge/./index.js'
  
SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/node_modules/bowser/es5.js'

I looked at this. Lodash import should now work in this PR

@richarddavison richarddavison merged commit 6dcd778 into main Nov 11, 2024
9 checks passed
@richarddavison richarddavison deleted the fix/cjs-export-default branch November 11, 2024 19:06
@nabetti1720
Copy link
Contributor

I looked at this. Lodash import should now work in this PR

Lodash and Lodash.merge seem to be different packages. Specifically, the situation is as follows.

import { Logger } from '@aws-lambda-powertools/logger';
console.log(Logger);
% llrt index17.js  
SyntaxError: Could not find export 'default' in module '/Users/shinya/Workspaces/llrt-test/node_modules/lodash.merge/./index.js'
  
% bun index17.js 
[class Logger]

Since the import does not produce a stack trace, I will report back after a little more verification by following the trace statement.

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