Use conditional exports

charlag commented 4 years ago

Description

This commit changes how exports are defined to handle different usage scenarios. Now build systems can detect what is required and node will pick appropriate version.

Motivation and Context

There are three different scenarios which require different imports: node with commonjs, node with es modules and browser (assumed to have es modules or be transpiled). Previously ospec would break if executed in a module context, build systems are also not able to fix that because require is used dynamically. issue #25

How Has This Been Tested?

I ran test-api with node 12. I was not able to run test-cli before or after the changes

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation change

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read all applicable contributing documents.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.
[x] I have updated the change log (if applicable).

charlag commented 4 years ago

I'm not sure if it's solves the original issue so maybe it's too early for review, sorry

charlag commented 4 years ago

I tried this alternatively:

    var inspect
    if (hasProcess) {
        import("util").then(function (util) {
            inspect = util.inspect
        })
    }

in the beginning of the file and it seems to work. It's not bulletproof. What do you think?

dead-claudia commented 4 years ago

Please confirm this also works with Rollup and Webpack, as some users do use this in the browser.

charlag commented 4 years ago

My second proposal in the comment works with Rollup, I need to check more browsers. About conditional exports: Rollup's plugin does not understand them yet but you can tell it to look into the module field of package.json and then it works.

I think that dynamic import as in the comment is a better solution to be honest.

dead-claudia commented 4 years ago

@charlag If you want to take a shot at that comment, feel free to. I'd definitely be open to considering it.

charlag commented 4 years ago

closing in favor of #27

MithrilJS / ospec