Skip to content

Conversation

@okwasniewski
Copy link
Contributor

Summary:

This PR adds build generated files to *ignore config files. This allows to locally run yarn lint

Changelog:

[INTERNAL] [ADDED] - Add build generated files to local config files

Test Plan:

CI Green

@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 Dec 7, 2023
@okwasniewski
Copy link
Contributor Author

cc: @huntie

@facebook-github-bot facebook-github-bot added p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 7, 2023
@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/react-native/Libraries/vendor/**/*
node_modules/
packages/*/node_modules
packages/*/dist
Copy link
Member

Choose a reason for hiding this comment

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

As a note, this hasn't been added globally because: 1/ not all of our packages follow this pattern yet, 2/ ideally (and open to debate!) even though we're in a monorepo, packages should self-specify their paths (e.g. in .gitignore, package.json#files), as if they were isolated.

However, I see there is a functional need to set this in .flowconfig and .prettierignore.

Requesting PR changes, to either:

  • Include .flowconfig and .prettierignore changes but drop this entry (personal preference but again open to debate).
  • Leave this entry included, but delete redundant .gitignore entries that are covered by this rule in each package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Packages that self-specify their ignore paths would be ideal however, eslint doesn't support that (eslint/eslint#13389, neither do prettier: prettier/prettier#4081).

If we delete this entry you are unable to run yarn lint locally, that's the output for me:

✖ 710 problems (18 errors, 692 warnings)
  18 errors and 655 warnings potentially fixable with the `--fix` option

And if we delete .gitignore from packages/ we will add dist folders to source control. If you don't want to duplicate this maybe we could remove packages .gitignore and move it to global .gitignore?

Copy link
Member

Choose a reason for hiding this comment

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

Ah my bad! I'm in line with the above 👍🏻👍🏻

I missed that this is .eslintignore and not .gitignore 😅

@analysis-bot
Copy link

analysis-bot commented Dec 7, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 17,738,212 +1,224,553
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 21,109,071 +1,224,822
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 38d07eb
Branch: main

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@github-actions
Copy link

github-actions bot commented Dec 8, 2023

This pull request was successfully merged by @okwasniewski in 1e0fc76.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Dec 8, 2023
Othinn pushed a commit to Othinn/react-native that referenced this pull request Jan 9, 2024
Summary:
This PR adds build generated files to *ignore config files. This allows to locally run `yarn lint`

## Changelog:

[INTERNAL] [ADDED] - Add build generated files to local config files

Pull Request resolved: facebook#41826

Test Plan: CI Green

Reviewed By: huntie

Differential Revision: D51939024

Pulled By: cortinico

fbshipit-source-id: cfd6c1c13dd23c692859cd06fa5955024fafc522
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 This PR has been merged. p: Callstack Partner: Callstack Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants