Remove proxy exports 🙌 #42
Conversation
|
Thanks for this! Should we also tacke the Icons here? |
|
I'd prefer to do it separately. We can however provide the named export WDYT? |
|
|
.npmignore
Outdated
| @@ -0,0 +1 @@ | |||
| *.story.js | |||
There was a problem hiding this comment.
Why do we need this? If both, .npmignore and .gitignore are present, then it will use only the .npmignore! So this change undoes the exclusion done by .gitignore
In simpler terms, npm prefers the .npmignore file if it is there, but will fall back to the .gitignore file. So if both are present, it will use the .npmignore file.
Source: https://stackoverflow.com/a/24942436
This also means that we would be exposing any sensitive files excluded from .gitignore as we'd be bundling them into the package as mentioned in For the love of god, don’t use .npmignore whose title I found pretty funny.
I'd use the files field instead, which we seem to be using as well?
There was a problem hiding this comment.
Ah good point. I just wanted to ignore stories to be published as well
There was a problem hiding this comment.
I’m not even sure it works at the moment because of npm/npm#11669. I’m on the phone now so I could not verify.
.npmignore
Outdated
| @@ -0,0 +1 @@ | |||
| *.story.js | |||
There was a problem hiding this comment.
Are we no longer publishing from dist by copying the package.json over or why did you feel that we need this? The stories shouldn't end up there in any case?
There was a problem hiding this comment.
Yes, we don't need to "publish from dist" anymore. It's just that we have some *.story.js files in the materials folder and I thought it would be good to not publish those
There was a problem hiding this comment.
Thanks for explaining. I googled a bit and this seems like the best option after all.
|
Yeah, let's do the |
emmenko
force-pushed
the
nm-remove-proxy-exports
branch
from
562ea2f to
a59bb6b
Compare
|
@dferber90 I changed it a bit, can you check again? 28cef77 |
|
To confirm:
|
| @@ -23,6 +25,8 @@ export { default as FieldLabel } from './components/field-label'; | |||
|
|
|||
| export { default as TextField } from './components/fields/text-field'; | |||
|
|
|||
| export { Icons }; | |||
There was a problem hiding this comment.
This is the new Icons export
There was a problem hiding this comment.
I wonder if it would be worth following up here with possibly code splitting this?
| @@ -23,6 +25,8 @@ export { default as FieldLabel } from './components/field-label'; | |||
|
|
|||
| export { default as TextField } from './components/fields/text-field'; | |||
|
|
|||
| export { Icons }; | |||
| // TODO: this type of export is deprecated and should be removed in the next major release | |||
| export * from './components/icons'; | |||
There was a problem hiding this comment.
This is kept for backwards compatibility and will be removed in the next major release
| @@ -68,3 +71,5 @@ export { default as Text } from './components/typography/text'; | |||
|
|
|||
| export { default as withMouseDownState } from './hocs/with-mouse-down-state'; | |||
| export { default as withMouseOverState } from './hocs/with-mouse-over-state'; | |||
|
|
|||
| export { i18n }; | |||
There was a problem hiding this comment.
This is also now its own named export
NOTE: the #40 should be merged first
Since we have almost migrated the imports in our codebase, we can remove the
proxy-exportsworkarounds in the next release.I also re-arranged a bit some other parts:
materialsis a top level folder and only contains*.mod.cssfilesimagesis now a top level folderconstraintsandspacingshave been moved tosrc/componentsimport { i18n } from '@commercetools-frontend/ui-kit;export