Skip to content
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

feat: set a default symbolicator.customizeFrame value #596

Conversation

motiz88
Copy link
Collaborator

@motiz88 motiz88 commented Jul 29, 2019

Summary:

Sets a default symbolicator.customizeFrame value. This configuration option was added to Metro in facebook/metro#435.

This is part of a redesign of facebook/react-native#24662.

Test Plan:

Equivalent config change tested as part of facebook/react-native#25839.

@motiz88 motiz88 force-pushed the symbolicator-customize-frame-defaults branch from 4cc46fe to 1a05e6e Compare July 29, 2019 11:21
@thymikee
Copy link
Member

Since this is a new config option, older Metro versions (used by RN 0.60) will spawn a validation warning, like this:

image

Which may confuse people.

Can you add a check to only include this if newer version of Metro is used?

@thymikee
Copy link
Member

Also, I'm so happy you work on this space to make errors more approachable ❤️

@motiz88
Copy link
Collaborator Author

motiz88 commented Jul 29, 2019

@thymikee Is there a scenario where people use a new CLI version with an old Metro version? Would it be enough to hold off on merging this until we cut a new Metro release and bump the dependency here?

@thymikee
Copy link
Member

If you could update Metro without breaking changes, then we could bump it internally here, provided it doesn't clash with RN. It still internally relies on 3 Metro packages: https://github.com/facebook/react-native/blob/59db059dbddb8101212f3739eecf0db494cfab41/package.json#L103-L105, which makes us (the CLI) to be up to date with what's there.

If you could also make it so that all Metro deps are required from the CLI (which makes sense), that would make us in charge of what's breaking or not. It would also make RN bundler-agnostic at the core. And in theory we could bump major Metro versions without a breaking change.

@cpojer
Copy link
Member

cpojer commented Jul 29, 2019

We are having quite a bunch of big changes in Metro related to changes in RN. We are hoping to bump Metro to 0.56 this week and update the CLI and RN. Could we merge this change as part of that? RN master/0.61 will only work with Metro 0.56 and above.

@thymikee
Copy link
Member

Ok, let's wait for Metro 0.56, merge this alongside and put 2.x into a branch for maintenance mode.

cc @Esemesek if you feel like moving some CLI stuff to TS, this is the time, as structural changes will be hard to port between branches.

@thymikee thymikee added this to the 3.x milestone Jul 29, 2019
motiz88 added a commit to motiz88/react-native that referenced this pull request Jul 29, 2019
Summary:
Changes `ExceptionsManager` to respect the `collapse` field in each symbolicated stack frame returned from Metro. This is ultimately driven by a Metro config option (which will have a default set in react-native-community/cli#596).

This is part of a redesign of work done originally in facebook#24662.

Reviewed By: cpojer

Differential Revision: D16500277

fbshipit-source-id: bded9a7b037b9f944b27125d43e35fcce5bd109a
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jul 29, 2019
Summary:
Pull Request resolved: #25839

Changes `ExceptionsManager` to respect the `collapse` field in each symbolicated stack frame returned from Metro. This is ultimately driven by a Metro config option (which will have a default set in react-native-community/cli#596).

This is part of a redesign of work done originally in #24662.

Reviewed By: cpojer

Differential Revision: D16500277

fbshipit-source-id: b0b035618cb000935a555796523637b5f0a688d3
@motiz88
Copy link
Collaborator Author

motiz88 commented Aug 14, 2019

With #630 now merged, I think we can merge this as well?

@thymikee
Copy link
Member

@motiz88 yep! Could you rebase and fix Flow?

@motiz88 motiz88 force-pushed the symbolicator-customize-frame-defaults branch from 1a05e6e to 41ac1e8 Compare August 14, 2019 11:51
@motiz88
Copy link
Collaborator Author

motiz88 commented Aug 14, 2019

Done. The failure in e2e-tests also exists on master: https://circleci.com/gh/react-native-community/cli/8256

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I'm aware of e2e failure, nothing to worry about. Thanks!

@thymikee thymikee merged commit 0292680 into react-native-community:master Aug 14, 2019
@thymikee
Copy link
Member

@motiz88 FYI this is available in v3.0.0-alpha.2

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 22, 2019
Summary:
Removes support for the non-standard `framesToPop` error property from React Native. Redboxes will now ignore this field. The way to skip uninformative frames in stack traces going forward is to use Metro's `customizeFrame` config option, for which the React Native CLI ships useful defaults (see: react-native-community/cli#596, react-native-community/cli#780)

Changelog: [General] [Removed] - Remove support for framesToPop from ExceptionsManager

Reviewed By: rickhanlonii

Differential Revision: D17877444

fbshipit-source-id: 04aa332c45ad35a99ae20e05fb87b34c91a557ab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants