jspm / jspm-cli

ES Module Package Manager
https://jspm.org
Apache License 2.0
3.79k stars 272 forks source link

Unable to create dew wrapper for this file. #2488

Closed soda-x closed 5 years ago

soda-x commented 5 years ago

https://dev.jspm.io/npm:regexpu-core@4.5.4/rewrite-pattern.dew.js

guybedford commented 5 years ago

Interestingly, this seems to work fine for me locally with jspm install actually. So I think it may be a CDN-specific bug. Will run some upgrades and see if that gets it fixed.

soda-x commented 5 years ago

Many thanks! @guybedford

soda-x commented 5 years ago

@guybedford

I have another two questions. For npm packages that expose pkg.module or pkg.jsnext, such as the mobx-react antd modules, this means I can get smaller, and more efficient bundles. But as https://dev.jspm.io/mobx-react or https://dev.jspm.io/antd shows, it handles pkg.main only.

Q1: I am curious about what the CDN transforming strategy is.

Q2: In the normal development process, I can the component as below

import {Button} from 'antd' but it is not gonna be worked by CDN involved.

WRONG:

import {Button} from 'https://dev.jspm.io/antd'

===>

OK:

import antd from 'https://dev.jspm.io/antd';
const { Button } = antd;
guybedford commented 5 years ago

@pigcan the "browser" field is supported but not "module" or "jsnext" as they are not compatible with the Node.js experimental modules implementation which is the correct semantics for es modules on npm.

I'm working on a proposal at https://github.com/guybedford/proposal-pkg-entries for a semantically correct way to do this kind of routing and am hoping to get buy-in from tooling and Node.js on that approach. jspm does support a non-standard "map" field though in the mean time.

In addition, named exports for CJS are not supported under Node.js experimental modules so we follow this too.

This friction will still be hit by build tooling in the next few months yet, so doesn't seem like a problem today but will be a general problem for everyone soon.

guybedford commented 5 years ago

So it turns out the underlying issue here is the following code path in rewrite-pattern.js:


const getUnicodePropertyValueSet = (property, value) => {
    const path = value ?
        `${ property }/${ value }` :
        `Binary_Property/${ property }`;
    try {
        return require(`regenerate-unicode-properties/${ path }.js`);
    } catch (exception) {
        throw new Error(
            `Failed to recognize value \`${ value }\` for property ` +
            `\`${ property }\`.`
        );
    }
};

where the CJS -> ESM conversion needs to be able to deduce ahead of time (because CJS is sync), all the possible values of regenerate-unicode-properties/x, which it can't do because of install time variations.

When jspm runs locally, is specially turns these cases into an internal _nodeRequire (which won't work in the browser), but the CDN will bail on them still.

I'm not sure what the best thing is to do here to be honest... because dynamic import is async, we'd effectively have to convert all the functions - which could have unintended consequences.

Unfortunately this may be a wontfix....

guybedford commented 5 years ago

I guess we could at least not fail and only fail on that execution path though... tracking this in https://github.com/jspm/babel-plugin-transform-cjs-dew/issues/8. Although this will likely still leave the package not fully functional.

guybedford commented 5 years ago

Ok, the file is building now, but will still have the limitations as described above.