-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Refactor API into new api
package
#3543
Merged
Merged
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
compulim
requested review from
a-b-r-o-w-n,
beyackle,
corinagum,
cwhitten,
srinaath,
tdurnford and
tonyanziano
as code owners
October 15, 2020 22:22
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
What's the |
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
corinagum
reviewed
Oct 19, 2020
corinagum
suggested changes
Oct 19, 2020
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.
Just a few questions/comments.
Since this is P1, we should wait to merge until all P0s are in.
corinagum
approved these changes
Oct 26, 2020
compulim
force-pushed
the
feat-api-package
branch
from
October 27, 2020 11:00
86b7278
to
438de16
Compare
corinagum
reviewed
Oct 27, 2020
@@ -18,7 +18,11 @@ const ErrorBox = ({ error, type }) => { | |||
<ScreenReaderText text={localize('ACTIVITY_ERROR_BOX_TITLE')} /> | |||
<div className={errorBoxStyleSet}> | |||
<div>{type}</div> | |||
<pre>{error.stack}</pre> | |||
{/* The callstack between production and development are different, thus, we should hide it for visual regression test */} |
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.
Suggested change
{/* The callstack between production and development are different, thus, we should hide it for visual regression test */} | |
{/* The callstack between production and development are different; therefore we should hide it for visual regression tests */} |
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.
Changelog Entry
Added
api
package, to be reused on React Native component, in PR #3543 by @compulimcore
->api
->component
(HTML-only) ->bundle
api
package. Some hooks are only available on the existingcomponent
package, due to their platform dependency or coupling with visual components. For example, Web Worker, 2D canvas,useMicrophoneButton*
are not available on theapi
packagecomponent
package due to their coupling with visual components or platform features. Some implementations, such as, card action middleware and activity grouping middleware, are available onapi
package. For example:component
package due to their coupling with their respective visual componentsimBack
,messageBack
andpostBack
actions are available onapi
package, butcall
,openUrl
and other platform-dependent actions are only available oncomponent
packageactivityMiddleware
,attachmentMiddleware
, etc, now support array for multiple middlewareDescription
This is for refactoring platform-neutral APIs into a new
api
package, to be reused on React Native component.Design
Please see #3316 for design.
In general, platform-neutral code are being moved to
api
package. Code that left on thecomponent
package assumes it is running inside a web browser.We also improved error handling, because
<ErrorBox>
is a visual component and is platform-dependent. We need to update the error boundary component and render<ErrorBox>
differently on different platforms.Although both React and React Native should support
styleSet
, the content could be fundamentally different. Thus,styleSet
are not offered inapi
packages, but incomponent
and the futurenative-component
package.Specific Changes
api
package, layered betweencore
andcomponent
api
packageapi
packageapi
packageapi
package, while keeping some in thecomponent
packageimBack
,messageBack
, andpostBack
, which are platform-neutral, are moved toapi
packagecall
,openUrl
, and others, are left untouched in thecomponent
packageapi
packageUserlandBoundary
to add error boundary to user components (e.g. components rendered via middleware)internalErrorBoxClass
prop for internal useapi
package can pass their own React component (e.g.component
package running in development mode)api
will render<ErrorBox>
when error caught in error boundary, which in turn, will use theinternalErrorBoxClass
to rendernode_env !== 'development
),component
will passundefined
tointernalErrorBoxClass
and will hide the<ErrorBox>
CHANGELOG.md
Review Checklist
Accessibility reviewed (tab order, content readability, alt text, color contrast)CSS styles reviewed (minimal rules, noz-index
)Internationalization reviewed (strings, unit formatting)package.json
andpackage-lock.json
reviewedSecurity reviewed (no data URIs, check for nonce leak)