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

boa: support esm #191

Merged
merged 1 commit into from May 18, 2020
Merged

boa: support esm #191

merged 1 commit into from May 18, 2020

Conversation

rickyes
Copy link
Contributor

@rickyes rickyes commented May 14, 2020

fixes: #173

Support export default and named export import.

/cc @yorkie

@CLAassistant
Copy link

CLAassistant commented May 14, 2020

CLA assistant check
All committers have signed the CLA.

@@ -0,0 +1,39 @@
import Boa from '../lib/index.js';
Copy link
Member

@yorkie yorkie May 14, 2020

Choose a reason for hiding this comment

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

Suggested change
import Boa from '../lib/index.js';
import boa from '../';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to automatically look for package.json.

Copy link
Member

Choose a reason for hiding this comment

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

Weird, we have defined the main as "lib/index.js" at https://github.com/alibaba/pipcook/blob/master/packages/boa/package.json#L5.

packages/boa/esm/loader.mjs Outdated Show resolved Hide resolved
@yorkie yorkie added the boa Python related issues label May 14, 2020
@yorkie
Copy link
Member

yorkie commented May 14, 2020

Thank you @rickyes I have commented some, and unit test is also required :)

Copy link
Collaborator

@Txiaozhe Txiaozhe left a comment

Choose a reason for hiding this comment

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

LGTM except lint nit:)

packages/boa/esm/loader.mjs Outdated Show resolved Hide resolved
@rickyes
Copy link
Contributor Author

rickyes commented May 14, 2020

Thank you @rickyes I have commented some, and unit test is also required :)

Of course, when the deconstruction import is supported, unit tests will be added together.

.github/workflows/build.yml Outdated Show resolved Hide resolved
packages/boa/esm/loader.mjs Outdated Show resolved Hide resolved
@rickyes rickyes changed the title [WIP] boa: support esm boa: support esm May 15, 2020
@rickyes
Copy link
Contributor Author

rickyes commented May 15, 2020

The named export is already supported, please review. @yorkie @Txiaozhe

Copy link
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

I have some comments for your changes, and we also need more tests, which needs cover builtins, base package and numpy.

@rickyes
Copy link
Contributor Author

rickyes commented May 15, 2020

I have some comments for your changes, and we also need more tests, which needs cover builtins, base package and numpy.

Okay, I'll supplement the base library test.

packages/boa/esm/loader.mjs Outdated Show resolved Hide resolved
@rickyes
Copy link
Contributor Author

rickyes commented May 18, 2020

I've made some changes, please review them.

@rickyes rickyes force-pushed the support-esm branch 3 times, most recently from b9697f6 to 2cacb74 Compare May 18, 2020 02:51
packages/boa/package.json Outdated Show resolved Hide resolved
@yorkie
Copy link
Member

yorkie commented May 18, 2020

@rickyes May I ask you to revert the code coverage? It seems that LGTM except that :)

@yorkie
Copy link
Member

yorkie commented May 18, 2020

internal/modules/run_main.js:54
    internalBinding('errors').triggerUncaughtException(
                              ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for /home/runner/.node-spawn-wrap-5008-e0ba0a808e26/node
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:65:15)
    at Loader.getFormat (internal/modules/esm/loader.js:113:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:244:31)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async Loader.import (internal/modules/esm/loader.js:178:17)
    at async internal/process/esm_loader.js:55:9 {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

It seems the above error occurred.

Copy link
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

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

LGTM

@yorkie yorkie merged commit 0d696b5 into alibaba:master May 18, 2020
@rickyes rickyes deleted the support-esm branch May 18, 2020 08:45
gindis pushed a commit to gindis/pipcook that referenced this pull request Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
boa Python related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

boa: use Node.js custom loader for better import-statement
4 participants