-
Notifications
You must be signed in to change notification settings - Fork 19
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: Multibuild to esm and commonjs #183
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert all changes made to example
folder as is not in the scope of this PR.
and there is an issue in packages/sdk/.npmignore
that needs to be addressed
ts configs look ok (by scanning them, dint go in deep)
packages/sdk/package.json
Outdated
"main": "./dist-cjs/index.js", | ||
"module": "./dist-esm/index.js", | ||
"source": "./src/index.ts", | ||
"types": "./types/src/index.d.ts", | ||
"exports": { | ||
".": { | ||
"import": "./dist-esm/index.js", | ||
"require": "./dist-cjs/index.js", | ||
"types": "./types/src/index.d.ts" | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How sure are you that is configured correctly?
(Asking because I know is a pain in the ass to do it correctly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issue that I have is with CRA. But that is because they use outdated webpack config. That's why I've updated all examples that are based on CRA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue with the globally Webpack or in the local setup?
Like the current latest webpack is 5.75.0 and it is pretty essential to work with it out of the box as have around ~50% of market-based on this statistics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is not related to webpack as a standalone tool, but rather pertains to the default configuration of webpack provided by CRA. It should be noted that our examples and UI do not utilize the default configuration of CRA, but instead employ craco
and react-app-rewired
due to the need for node polyfills and etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will not block merge, if someone has an objection please leave it, will give second approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wainola build si broken please fix it
962d939
to
ce57610
Compare
Conflicts resolved, lint error fixed. @BeroBurny @wainola |
Description
Related Issue Or Context
Closes: #184
How Has This Been Tested? Testing details.
Types of changes
Checklist: