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

fix: export PERMISSIONS #628

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Conversation

1natsu172
Copy link
Contributor

@1natsu172 1natsu172 commented Jul 4, 2021

Summary

The PERMISSIONS that were exported in v2 are not exported in v3. Specifically, the PERMISSIONS object is exported, but the actual object is empty.

This causes an error when migrating from v2 because the permission constants are not found.

In this PR, the PERMISSIONS exported from each OS file has been removed and integrated into permission.ts.

Test Plan

  • Check the PERMISSIONS exported from each OS file is not used from anywhere.
  • ran
    • yarn run lint
    • yarn run format
    • yarn run tscheck

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

@1natsu172 1natsu172 marked this pull request as ready for review July 4, 2021 12:00
@1natsu172 1natsu172 requested a review from zoontek as a code owner July 4, 2021 12:00
@zoontek
Copy link
Owner

zoontek commented Jul 4, 2021

@1natsu172 I'm not sure to understand what this PR really fix. Exporting empty maps on other platforms is something we want to avoid bundling unnecessary permissions.

Ex: The permission list on windows weights ~8ko. There's no need to put that in the iOS or the Android JS bundle.

@1natsu172
Copy link
Contributor Author

1natsu172 commented Jul 5, 2021

@zoontek Sorry about that. I've been using the constants of each OS for test assertions in jest.

I meant that the automatic resolution of "platform-specific extensions" by metro is not possible in jest, so can't get the constants in the node runtime. So I tried to export it with this PR.

However, I also really understood your point about not doing unnecessary platform bundling.

What do you think of the idea of not exporting from src/permissions.ts but exporting the constants for each platform for testing from mock.js? If you agree, will modify this PR.

@zoontek
Copy link
Owner

zoontek commented Jul 5, 2021

@1natsu172 I agree, only the mock needs to be edited.

// mock.js
const {PERMISSIONS: {ANDROID}} = require('./dist/commonjs/permissions.android');
const {PERMISSIONS: {IOS}} = require('./dist/commonjs/permissions.ios');
const {PERMISSIONS: {WINDOWS}} = require('./dist/commonjs/permissions.windows');

const {RESULTS} = require('./dist/commonjs/results');

const PERMISSIONS = {ANDROID, IOS, WINDOWS};
export {PERMISSIONS, RESULTS};

// …

This should work. I don't want to export ANDROID, IOS or WINDOWS directly from the module, as this makes no sense / it's no clear to use in a codebase.

@1natsu172 1natsu172 force-pushed the export-permission branch from 1b30da8 to aa338bb Compare July 5, 2021 09:20
@1natsu172
Copy link
Contributor Author

@zoontek Thank you. I just changed the PR content to mock.js and re-committed. Please check it.

@zoontek
Copy link
Owner

zoontek commented Jul 5, 2021

LGTM 👍

@zoontek zoontek merged commit eeedc79 into zoontek:master Jul 5, 2021
@zoontek
Copy link
Owner

zoontek commented Jul 5, 2021

I made a new release for it: https://github.com/zoontek/react-native-permissions/releases/tag/3.0.5

@1natsu172
Copy link
Contributor Author

@zoontek Thanks for the very very fast release! 🚀

@1natsu172 1natsu172 deleted the export-permission branch July 5, 2021 10:58
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.

2 participants