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 $projectRoot/package.json from assets #420

Closed
wants to merge 2 commits into from
Closed

Exclude $projectRoot/package.json from assets #420

wants to merge 2 commits into from

Conversation

robertying
Copy link
Contributor

Summary

From dcb41e3#diff-235a3e5d21175615e1cc254dd8b17eb2, files with json extension will be considered as assets and returned as outputAssets for @react-native-community/cli.

This means cli will copy files with json extension to resource dir, in Android's case, android/app/build/res.

Here comes a problem. If package.json at the project root gets copied, it won't be renamed as other package.json in node_modules (e.g. raw/node_modules_reactnativecodepush_package.json). Instead, it will reside in res as package.json, which causes the error:

facebook/react-native#25325

android/app/src/main/res/raw/package.json: Error: package is not a valid resource name (reserved Java keyword)

This regression (I believe ;)) was introduced to React Native 0.60.0-rc with metro bumping from 0.51 to 0.54.

Possible impact

  1. Typical usage concerning package.json at the root will be something like import packageJson from "./package.json" in JS files to get some project configurations and those content will be bundled into index.android.bundle no matter what. So removing $projectRoot/package.json won't have too much impact.

  2. This new (or bad?) behavior has only been in React Native 0.60-rc so it won't affect current projects running 0.59. It should be fixed before the final release of 0.60, I assume.

Test plan

  1. react-native init resoucesRepro --version react-native@next (so it uses 0.60.0-rc)
  2. Add import packageJson from './package.json' to App.js.
  3. cd android && ./gradlew assembleRelease
  4. Get the error.
  5. Modify node_modules/metro/src/DeltaBundler/Serializers/getAssets.js to match this commit.
  6. cd android && ./gradlew assembleRelease
  7. It now works!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 21, 2019
@codecov-io
Copy link

Codecov Report

Merging #420 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #420   +/-   ##
=======================================
  Coverage   83.21%   83.21%           
=======================================
  Files         181      181           
  Lines        6089     6089           
  Branches      964      964           
=======================================
  Hits         5067     5067           
  Misses        891      891           
  Partials      131      131
Impacted Files Coverage Δ
...es/metro/src/DeltaBundler/Serializers/getAssets.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7df3e79...9996256. Read the comment docs.

@bneigher
Copy link

please merge!

@IgorBelyayev
Copy link

IgorBelyayev commented Jul 28, 2019

Temporary fix while we wait for this to be merged.

Add this script to the root project directory and run it postinstall:

// fix_metro_android_release_bug.js
// Temporary fix for this issue: https://github.com/facebook/metro/pull/420
const fs = require('fs');

const fileLocation = './node_modules/metro/src/DeltaBundler/Serializers/getAssets.js';
const targetText = 'getJsOutput(module).type === "js/module/asset"';
const replacementText = 'getJsOutput(module).type === "js/module/asset" && path.relative(options.projectRoot, module.path) !== "package.json"';

const fileContent = fs.readFileSync(fileLocation, 'utf8');
if (fileContent.includes(targetText) && !fileContent.includes(replacementText)) {
    const patchedFileContent = fileContent.replace(targetText, replacementText);
    fs.writeFileSync(fileLocation, patchedFileContent, 'utf8');
}

Then, inside your inside your package.json:

  "scripts": {
    "postinstall": "babel-node ./fix_metro_android_release_bug.js",
    ...
  }

The script applies the changes from this PR.

@bneigher
Copy link

@IgorBelyayev love it ahaha

@christiangarciareyes
Copy link

Solución temporal mientras esperamos que esto se fusione.

Agregue este script al directorio raíz del proyecto y ejecútelo postinstall:

// fix_metro_android_release_bug.js
// Temporary fix for this issue: https://github.com/facebook/metro/pull/420
const fs = require('fs');

const fileLocation = './node_modules/metro/src/DeltaBundler/Serializers/getAssets.js';
const targetText = 'getJsOutput(module).type === \'js/module/asset\'';
const replacementText = 'getJsOutput(module).type === \'js/module/asset\' && path.relative(options.projectRoot, module.path) !== \'package.json\'';

const fileContent = fs.readFileSync(fileLocation, 'utf8');
if (fileContent.includes(targetText) && !fileContent.includes(replacementText)) {
  const patchedFileContent = fileContent.replace(targetText, replacementText);
  fs.writeFileSync(fileLocation, patchedFileContent, 'utf8');
}

Entonces, dentro de tu interior tu package.json:

  "scripts": {
    "postinstall": "babel-node ./fix_metro_android_release_bug.js",
    ...
  }

El script aplica los cambios de este PR.

disculpa, cual seria la raiz de mi proyecto???

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

👍 Thank you.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Oops, wrong button.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gmaclennan added a commit to digidem/mapeo-mobile that referenced this pull request Aug 5, 2019
@facebook-github-bot
Copy link
Contributor

@cpojer merged this pull request in f3c9862.

@facebook-github-bot
Copy link
Contributor

@cpojer merged this pull request in f3c9862.

hawkrives added a commit to StoDevX/AAO-React-Native that referenced this pull request Sep 8, 2019
@robertying
Copy link
Contributor Author

@cpojer this commit does not seem to be included in the latest release (0.56.0)'s npm package. Can you please take a look into this? Thanks!

@cdunkel
Copy link

cdunkel commented Sep 26, 2019

I'm also on 0.56.0 and still seeing this issue. Any updates?

@cdunkel
Copy link

cdunkel commented Sep 26, 2019

@robertying, I had to update to RN 0.61.x in order to pick up this change, if that helps.

@robertying
Copy link
Contributor Author

@cdunkel thanks! I think 0.61 now includes this patch.

@robertying robertying deleted the fix-assets branch September 27, 2019 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants