refactor: more @superset-ui/core to superset-ui #728
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/ok7wmdq4r |
ktmud
force-pushed
the
monopackage-move-core
branch
from
a6f0657
to
e1e5c77
Compare
| @@ -1,5 +1,5 @@ | |||
| import React, { PureComponent } from 'react'; | |||
| import { isDefined } from '@superset-ui/core'; | |||
| import { isDefined } from 'superset-ui/lib/core'; | |||
There was a problem hiding this comment.
Because of restrictions from nimbus, currently we still have to import from the superset-ui/lib folder. I really want to get rid of the /lib folder for published packages, but it would require more advanced customization with the build process that is not easy to do with nimbus.
Will try to revisit this after all packages are merged.
There was a problem hiding this comment.
I don't really understand enough about nimbus to grok these restrictions. Does this also mean we'll have to import from /lib in Superset?
There was a problem hiding this comment.
Because of restrictions from nimbus
What restriction? I don't remember having that issue before.
There was a problem hiding this comment.
I really want to get rid of the /lib folder for published packages
Could you elaborate more on the motivation to remove lib?
There was a problem hiding this comment.
What restriction? I don't remember having that issue before.
nimbus expects the src and outputs to be certain layout and it's difficult to override the defaults.
package.json can specify only one main script, so while you can import lib/index.js directly from "@superset-ui/ui`:
import { abc } from "@superset-ui/abc"You cannot use import { abc } from "superset-ui/abc" to import from superset-ui/lib/abc/index.js
There was a problem hiding this comment.
I don't really understand enough about nimbus to grok these restrictions. Does this also mean we'll have to import from
/libin Superset?
For the time-being or before the packages are all merged, yes. We can always merge the feature branch only when we find a solution to get rid of /lib.
It'd be possible to import all of them from the root index file though.
import { SupersetChart } from "superset-ui"There was a problem hiding this comment.
I really want to get rid of the /lib folder for published packages
Could you elaborate more on the motivation to remove
lib?
It just looks better. 😄 Plus it'd also be easier (and less confusing) when we redirect it to the /src folder via resolve aliases, for IDE intellisense or npm link.
But I guess we can live with it if it's too much work or adds too much complexity to the build process.
There was a problem hiding this comment.
Makes sense. This looks pretty good to me:
import { SupersetChart } from "superset-ui"
Though it may be too many things exported under one namespace.
There was a problem hiding this comment.
Makes sense. This looks pretty good to me:
import { SupersetChart } from "superset-ui"Though it may be too many things exported under one namespace.
I think we should export all under one. Seems a lot simpler.
There is also no naming collision at the moment.
(I previously did re-export all subpackages in @superset-ui/superset-ui that re-exports all from the micro packages and there was no name conflicts)
Codecov Report
@@ Coverage Diff @@
## monopackage #728 +/- ##
============================================
Coverage 24.35% 24.35%
============================================
Files 340 340
Lines 7637 7637
Branches 929 929
============================================
Hits 1860 1860
Misses 5704 5704
Partials 73 73
Continue to review full report at Codecov.
|
Better use |
Sure. I'll use |
|
We'll just repurpose |
🏠 Internal
Move
@superset-ui/coreto a new package calledsuperset-ui. This is the first step of merging all@superset-ui/core packages into one package.Merges into a feature branch
monopackage.