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

Exclude unnecessary file and folders from release distribution #114

Merged
merged 1 commit into from
Nov 13, 2019
Merged

Exclude unnecessary file and folders from release distribution #114

merged 1 commit into from
Nov 13, 2019

Conversation

SaeedZhiany
Copy link
Contributor

all Gradle wrapper files need to be excluded from distribution

I already have the folder in my project that used this library as a dependency.

image

so I add the folder into .npmignore to exclude it from release distributions.

you can take a look at the discussion on this PR

cc @friederbluemle

Copy link
Contributor

@friederbluemle friederbluemle left a comment

Choose a reason for hiding this comment

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

Nice @SaeedZhiany - Yes, the jar file should not be distributed. In addition to gradle/, you can also consider excluding the following files. They are all part of (or generated by) a Gradle wrapper distribution:

.gradle/
build/
gradlew
gradlew.bat
local.properties

@SaeedZhiany
Copy link
Contributor Author

SaeedZhiany commented Nov 12, 2019

@friederbluemle
Is your suggested above list enough? I saw your PR with more ignored items

*.iml
.DS_Store
.gradle/
.idea/
.npmignore
build/
gradle/
gradlew
gradlew.bat
local.properties

@SaeedZhiany SaeedZhiany changed the title Exclude Android gradle folder Exclude unnecessary file and folders from release distribution Nov 12, 2019
@jdnichollsc
Copy link
Member

Let me know when this PR is OK for you guys, thanks for your help! 💯

@friederbluemle
Copy link
Contributor

@SaeedZhiany - You would need to either put these entries into a separate .npmignore file inside of android/, or prefix all android specific entries with android/. Otherwise they have no effect.

@SaeedZhiany
Copy link
Contributor Author

@friederbluemle
Oh, you're right, thanks.
is there any file that must be ignored in the ios folder?

# root Directory
.github/
example/
.npmignore

# Android
android/*.iml
android/.gradle/
android/.idea/
android/build/
android/gradle/
android/gradlew
android/gradlew.bat
android/local.properties

is it OK?

@jdnichollsc
which one do you prefer? having multiple .npmignore files in separated folders (root, android) or aggregate all of them in the root directory of the repository?

@friederbluemle
Copy link
Contributor

Ideally, you'd also want to add a files section in package.json with the files and folders to include in the release distribution. Once you do that, having only one .npmignore won't work anymore.

For this repository, the recommended setup would be:

// package.json
...
  "main": "index.js",
  "files": [
    "android/",
    "ios/",
    "index.d.ts",
    "RNInAppBrowser.podspec"
  ],
...
// .npmignore
example/
// android/.npmignore
*.iml
.DS_Store
.gradle/
.idea/
.npmignore
build/
gradle/
gradlew
gradlew.bat
local.properties
// ios/.npmignore
*/project.xcworkspace/
*/xcuserdata/
.DS_Store
.npmignore
Pods/
build/

@SaeedZhiany
Copy link
Contributor Author

@friederbluemle
is it not necessary to include index.js in "files" too?

@friederbluemle
Copy link
Contributor

Good question - Since index.js is set as main, it is not necessary to also include it in files. Along with several other files (e.g. package.json and README.md) it is always included in the archive.

@friederbluemle
Copy link
Contributor

friederbluemle commented Nov 13, 2019

Btw, you can remove .github/ and .npmignore from the root .npmignore. In the root directory, only the entries listed under files in package.json (along with other defaults) are included.

@friederbluemle
Copy link
Contributor

To test what will be included, just run:

npm pack --dry-run

@SaeedZhiany
Copy link
Contributor Author

Thanks, @friederbluemle! I learn more new things every day from you 👍

@SaeedZhiany
Copy link
Contributor Author

SaeedZhiany commented Nov 13, 2019

@jdnichollsc
I think it's OK to merge now.

this is the list of all files that will be package after merging this PR.

image

Copy link
Contributor

@friederbluemle friederbluemle left a comment

Choose a reason for hiding this comment

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

I tested this locally, everything looks great. Ready to merge @jdnichollsc 👍

@jdnichollsc jdnichollsc merged commit bd5282e into proyecto26:master Nov 13, 2019
@SaeedZhiany SaeedZhiany deleted the patch-1 branch November 13, 2019 20:00
@jdnichollsc
Copy link
Member

thanks for your help guys! 🥇

@SaeedZhiany
Copy link
Contributor Author

Please also consider to release a new version.

@jdnichollsc
Copy link
Member

jdnichollsc commented Nov 13, 2019

give me a moment please 👍

@friederbluemle
Copy link
Contributor

@jdnichollsc - Could you also merge #115 before the release? Even more unnecessary files will be excluded.

phuongwd pushed a commit to phuongwd/react-native-inappbrowser that referenced this pull request Jul 31, 2020
Exclude unnecessary file and folders from release distribution
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.

None yet

3 participants