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

Adding exports map and peerDependencies to package.json #138

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JayaKrishnaNamburu
Copy link

Adding exports map and peerDependencies to package.json

Currently when we use antd from any cdn provider like skypack or antd we will run into 2 issues regarding the cdn you are using. I tried to resolve the issues in this PR.

Missing peerDependencies

Currently i see the package is using react and react-dom but they are defined only in devDependencies alone. This doesn't raise a issue when used local, because most of the time. It will be used in a react based project and works. But when loaded via cdn's like this. They miss out the package information and breaks instead.

How jspm handles the issue

Right now, if the package is missing in package.json it tries to redirect to the latest version of it.
It's just as a fallback and here it is why it can't the solution always and the package need to be corrected, for example you are using react@16.2 in development. But it might load react16.7 since it is the latest release.
And that's the main reason why skypack throws error too.

How skypack handles it

Skypack throws an error instead, currently it checks on peerDependencies and dependencies alone for the version number.

Missing export map

Both the cdn's highly rely on exports field in the package.json for resolving the modules which is a node standard for handling. Since main field is missing from the package.json and exports map too. It is causing issues when static analysis happens in the package.

But the exports introduces the change in the way the package is being used. For example, rc-image which uses this package for adding functionalities uses like

https://github.com/react-component/image/blob/ac08b58b28a560ce2422ad43ac61a1f664f3c602/src/getFixScaleEleTransPosition.tsx#L1

import raf from 'rc-util/lib/raf';
import { getClientSize } from 'rc-util/lib/Dom/css';
import addEventListener from 'rc-util/lib/Dom/addEventListener';
import { getOffset } from 'rc-util/lib/Dom/css';

should be changed to

import raf from 'rc-util/raf';
import { getClientSize } from 'rc-util/Dom/css';
import addEventListener from 'rc-util/Dom/addEventListener';
import { getOffset } from 'rc-util/Dom/css';

Since the cjs or esm resolution is taken care by export map itself.

PS:
If the package doesn't want to introduce this change for imports, i can revert the change from the PR and add peerDependency change alone.

@coveralls
Copy link

coveralls commented Aug 26, 2020

Coverage Status

Coverage remained the same at 29.129% when pulling 29c46f0 on JayaKrishnaNamburu:chore/peer-dependencies into 458d5a5 on react-component:master.

@JayaKrishnaNamburu
Copy link
Author

I just made an update with the commit --> 29c46f0 this will not introduce any breaking changes in the way the package is being currently used 😄

@brandonbloom
Copy link
Contributor

Adding these peerDependencies is a good fix, but there is follow-up work to be done as well. See react-component/animate#99 (comment)

@JayaKrishnaNamburu
Copy link
Author

Yes @brandonbloom this is going to fix only a specific case with rc-image but still have the same issue with other components. But i don't see any discussion going around this for some time.

@yoyo837
Copy link
Member

yoyo837 commented Oct 30, 2020

Conflicted!

1 similar comment
@afc163
Copy link
Member

afc163 commented Nov 17, 2020

Conflicted!

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.

None yet

5 participants