Tech/api package #5402
Merged
Tech/api package #5402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ndelangen
requested review from
tmeasday,
pksunkara,
alexandrebodin,
Hypnosphi and
apastuhov
ndelangen
requested review from
dangreenisrael,
igor-dv and
theinterned
as code owners
ndelangen
commented
Jan 29, 2019
ndelangen
commented
Jan 29, 2019
ndelangen
commented
Jan 29, 2019
ndelangen
requested review from
alterx,
danielduan,
Gongreg,
thani-sh and
thomasbertet
as code owners
Codecov Report
@@ Coverage Diff @@
## next #5402 +/- ##
==========================================
- Coverage 38.04% 37.93% -0.11%
==========================================
Files 643 645 +2
Lines 9420 9626 +206
Branches 1344 352 -992
==========================================
+ Hits 3584 3652 +68
- Misses 5268 5927 +659
+ Partials 568 47 -521
Continue to review full report at Codecov.
|
|
This will hopefully help restore/simplify some of the ondevice addons. ondevice-backgrounds is mostly broken at the moment because it's still expecting data coming through the websocket when some of the values are in state. |
# Conflicts: # addons/a11y/package.json # addons/actions/package.json # addons/backgrounds/package.json # addons/cssresources/package.json # addons/cssresources/src/css-resource-panel.tsx # addons/notes/package.json # app/react-native-server/package.json # app/react-native-server/src/client/manager/provider.js # examples-native/crna-kitchen-sink/package.json # lib/addons/package.json # lib/ui/package.json # yarn.lock
# Conflicts: # addons/a11y/package.json # addons/actions/package.json # addons/backgrounds/package.json # addons/cssresources/package.json # addons/notes/package.json # app/react-native-server/package.json # lib/addons/package.json # lib/api/src/modules/versions.ts # lib/ui/package.json # lib/ui/src/core/context.js
|
Seems these tests may be leaking memory:
|
…bug && CLEANUP tests log
|
Would like to investigate why some tests take so long |
ndelangen
force-pushed
the
tech/api-package
branch
from
fee3a5e to
62299a6
Compare
# Conflicts: # yarn.lock
ndelangen
commented
Mar 18, 2019
| @@ -264,7 +264,7 @@ jobs: | |||
| at: . | |||
| - run: | |||
| name: Test | |||
| command: yarn test --coverage --runInBand --core | |||
| command: yarn test --coverage --w2 --core | |||
There was a problem hiding this comment.
This speeds up the processing of coverage a lot. It's limited to 2 CPUs since that seems to work on CIs best.
ndelangen
commented
Mar 18, 2019
| @@ -1,7 +1,7 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`addon Info should render <Info /> and external markdown 1`] = ` | |||
| <deprecated> | |||
| <Component> | |||
There was a problem hiding this comment.
Side-effect of muting the deprecation messages when running tests
ndelangen
commented
Mar 18, 2019
| componentWillUnmount() { | ||
| const { api } = this.props; | ||
| api.off(STORY_RENDERED, this.onStoryChange); | ||
| const NotesPanel = ({ active }: Props) => { |
There was a problem hiding this comment.
Addon became state-less 🎉
ndelangen
commented
Mar 19, 2019
| ], | ||
| snapshotSerializers: ['jest-emotion', 'enzyme-to-json/serializer'], | ||
| coverageDirectory: 'coverage', | ||
| testEnvironment: 'jsdom', | ||
| coverageReporters: ['lcov'], |
There was a problem hiding this comment.
This seems to improve the coverage report massively as well
ndelangen
commented
Mar 19, 2019
| warning, | ||
| }); | ||
| return deprecatedFn; | ||
| jest.mock('util-deprecate', () => { |
There was a problem hiding this comment.
deprecate is already mocked, but this changes the implementation
ndelangen
commented
Mar 19, 2019
| @@ -1,7 +1,7 @@ | |||
| import { navigator } from 'global'; | |||
|
|
|||
| // The shortcut is our JSON-ifiable representation of a shortcut combination | |||
| type Shortcut = string[]; | |||
| import { KeyCollection, Event } from '../modules/shortcuts'; | |||
There was a problem hiding this comment.
@Jessica-Koch I'm hoping I did the merges correctly, It seems to all work. I hope this is in line with how you want it.
It's all in typescript now 🎉
|
Omg
Awesome!!!!
…Sent from my iPhone
On Mar 18, 2019, at 5:03 PM, Norbert de Langen ***@***.***> wrote:
@ndelangen commented on this pull request.
In lib/api/src/lib/shortcut.ts:
> @@ -1,7 +1,7 @@
import { navigator } from 'global';
// The shortcut is our JSON-ifiable representation of a shortcut combination
-type Shortcut = string[];
+import { KeyCollection, Event } from '../modules/shortcuts';
@Jessica-Koch I'm hoping I did the merges correctly, It seems to all work. I hope this is in line with how you want it.
It's all in typescript now 🎉
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
benoitdion
added a commit
that referenced
this pull request
Mar 20, 2019
Following up discussions in #5402, this a proposal for what a typed channel api could look like. Communication via channel is core to the storybook architecture. Adding type safety should help prevent bug and help improve code discovery for new contributors.
This was referenced Mar 20, 2019
Hypnosphi
reviewed
Mar 27, 2019
| const checkDeprecatedThemeOptions = options => { | ||
| if (Object.keys(deprecatedThemeOptions).find(key => !!options[key])) { | ||
| const checkDeprecatedThemeOptions = (options: Options) => { | ||
| if (Object.values(deprecatedThemeOptions).find(v => !!v)) { |
There was a problem hiding this comment.
What is this check intended to do? Looks like it just checks whether deprecatedThemeOptions has some values, which is always true
There was a problem hiding this comment.
it should map deprecated options to themeVars
name: "brandTitle";
url: "brandUrl";
There was a problem hiding this comment.
I mean that
if (Object.values({name: 'brandTitle', url: 'brandUrl'}).find(v => !!v))
evaluates to
if (['brandTitle', 'brandUrl'].find(v => !!v))
// same as
if ('brandTitle')
// same as
if (true)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: Accessing state and API's is kinda bothersome right now.
This extracts the ManagerProvider & ManagerConsumer into a package called
@storybook/api.The consumer can now be used in other places.
Also I migrated most of it to TypeScript