Separate Package for `@react-native/event-emitter` #34401

yungsters · 2022-08-12T22:08:05Z

Summary

Moves EventEmitter.js (and support files) into packages/event-emitter and prepares it to be published as an independent package named @react-native/event-emitter.

Changelog

[General][Changed] - Hoisted EventEmitter into a separate package, @react-native/event-emitter.

Test Plan

$ cd react-native/packages/event-emitter
$ yarn
$ yarn test

github-actions · 2022-08-12T22:09:02Z

	Fails
🚫	`node` failed.

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Log

Error:  Error: Failed to load parser '@typescript-eslint/parser' declared in '../.eslintrc.js » @react-native-community/eslint-config#overrides[1]': Cannot find module 'typescript'
Require stack:
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/parser.js
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/index.js
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/parser.js
- /home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/index.js
- /home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Function._module2.default._load (/home/runner/work/react-native/react-native/bots/node_modules/override-require/dist/overrideRequire.js:43:25)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/parser.js:35:25)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Object.customModuleHandler (/home/runner/work/react-native/react-native/bots/node_modules/danger/distribution/runner/runners/inline.js:129:28)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/parser.js',
    '/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/typescript-estree/dist/index.js',
    '/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/parser.js',
    '/home/runner/work/react-native/react-native/node_modules/@typescript-eslint/parser/dist/index.js',
    '/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs'
  ]
}
danger-results://tmp/danger-results.json

Generated by 🚫 dangerJS against 2037697

analysis-bot · 2022-08-12T22:39:44Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,611,839	-71
android	hermes	armeabi-v7a	7,026,432	-42
android	hermes	x86	7,911,491	-56
android	hermes	x86_64	7,885,392	-64
android	jsc	arm64-v8a	9,490,481	-62
android	jsc	armeabi-v7a	8,267,943	-40
android	jsc	x86	9,427,717	-50
android	jsc	x86_64	10,020,719	-57

Base commit: be8fe7a
Branch: main

analysis-bot · 2022-08-12T23:19:21Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 4eec473
Branch: main

yungsters · 2022-08-12T23:40:00Z

I suppose this is something worth discussing in react-native-community/discussions-and-proposals#480, but the expectations for a package inside react-native/packages is pretty murky. For instance…

Should I separately configure Flow (and include flow-bin in devDependencies)? ESLint? Jest? TypeScript? Babel?
How should I verify that @react-native/event-emitter references in react-native work correctly, without having to publish merge a pull request (which might break things), publish @react-native/event-emitter, and then re-test locally? I think that's what Yarn Workspaces is supposed to be for… but what are the right steps to verify?
My intuition is that npm packages should be compiled to run in as many environments as possible, but for something like this… is that really necessary? It's a nice to have, but React Native provides some nice assumptions (e.g. Metro support for ES Modules and Flow). Should I be manually producing X.js that has Flow removed via Babel, but also include X.js.flow? It would be easier if X.js could just be the same file in the react-native repository source.

Apologies for the stream of consciousness. 😅

And also… I have no idea what is going on with Danger and the ESLint dependency error:

Error:  Error: Failed to load parser '@typescript-eslint/parser' declared in '../.eslintrc.js » ./packages/eslint-config-react-native-community/index.js#overrides[1]': Cannot find module '@typescript-eslint/parser'
Require stack:
- /home/runner/work/react-native/react-native/packages/eslint-config-react-native-community/index.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.resolve (node:internal/modules/cjs/helpers:108:19)
    at Object.resolve (/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:2325:46)
    at ConfigArrayFactory._loadParser (/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3288:39)
    at ConfigArrayFactory._normalizeObjectConfigDataBody (/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3062:43)
    at _normalizeObjectConfigDataBody.next (<anonymous>)
    at ConfigArrayFactory._normalizeObjectConfigData (/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3003:20)
    at _normalizeObjectConfigData.next (<anonymous>)
    at ConfigArrayFactory._normalizeObjectConfigDataBody (/home/runner/work/react-native/react-native/node_modules/@eslint/eslintrc/dist/eslintrc.cjs:3096:25)
    at _normalizeObjectConfigDataBody.next (<anonymous>) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/runner/work/react-native/react-native/packages/eslint-config-react-native-community/index.js'
  ]
}
danger-results://tmp/danger-results.json

packages/event-emitter/package.json

yungsters · 2022-08-15T16:58:12Z

I investigated the Danger error a bit. It appears that this PR uncovered a dormant problem with our ESLint setup (wherein the dependencies of @react-native-community/eslint-config are not installed and resolvable).

I suspect that this problem has been dormant because most of the React Native repository has not used TypeScript, so all of the optional TypeScript dependencies were blissfully absent. Since this PR introduces TypeScript definition files to be linted, those code paths are now being exercised and the TypeScript dependencies cannot resolve, therefore causing errors in the ESLint pipeline.

#34423 reworks how we extend @react-native-community/eslint-config and should resolve the Danger error here.

…#34423) Summary: Changes the React Native base ESLint configuration to consume `react-native-community/eslint-config` as a [yarn workspace](https://classic.yarnpkg.com/lang/en/docs/workspaces/), so that any dependencies of `react-native-community/eslint-config` can also be resolved from the root directory of `react-native`. Previously, `~/.eslintrc.js` extended `react-native-community/eslint-config` using a relative file path. This is problematic because if any dependencies (notably, optional peer dependencies such as some of the TypeScript dependencies) are not already installed at the root directory of `react-native`, running ESLint could fail to resolve any required dependencies. In other words, there was an implicit dependency that `react-native/yarn.lock` would also contain any dependencies required by `react-native/packages/eslint-config-react-native-community/yarn.lock`. With this change, running `yarn` from the root directory of `react-native` will also install any dependencies of `react-native-community/eslint-config`, and it will also symlink `react-native/node_modules/react-native-community/eslint-config` to `../../packages/eslint-config-react-native-community` (meaning any local changes to the config will still be reflected during active development). ## Changelog [Internal] Pull Request resolved: #34423 Test Plan: Successfully install dependencies and run ESLint. ``` $ cd react-native $ yarn $ yarn lint ``` Successfully run Danger on #34401 (which would previously fail because `typescript-eslint/eslint-plugin` could not be resolved from the root directory of `react-native`): ``` $ cd react-native/bots $ yarn $ node ./node_modules/.bin/danger pr #34401 ``` Reviewed By: NickGerleman Differential Revision: D38710285 Pulled By: yungsters fbshipit-source-id: a06ceea0884a90be60f6f5db9a5d42be52a951d5

yungsters · 2022-08-16T02:52:54Z

packages/event-emitter/scripts/build.sh

+babel "$ROOT_DIR/src/EventEmitter.js" --out-dir "$ROOT_DIR/lib"
+prettier "$ROOT_DIR/lib/EventEmitter.js" --write
+
+cp "$ROOT_DIR/src/EventEmitter.js" "$ROOT_DIR/lib/EventEmitter.js.flow"
+cp "$ROOT_DIR/src/EventEmitter.d.ts" "$ROOT_DIR/lib/EventEmitter.d.ts"


A potentially interesting choice that I've made here is checking in the build artifacts at packages/event-emitter/lib.

The reason I am doing this is because in order for @react-native/event-emitter to be resolvable locally using Yarn Workspaces without requiring that developers independently run cd packages/event-emitter; yarn (which is cumbersome), we can instead check these built artifacts in so that rebasing onto the main branch simply gets you the newly built files. That is, the onus of updating these files is on whoever changes (and presumably tests) the source files.

As a consequence, one strange thing I'm doing here is running prettier on the Babel output file. This is so that yarn run format-check does not fail on that build artifact in CI. 🤷‍♂️

A potentially interesting choice that I've made here is checking in the build artifacts at packages/event-emitter/lib.

FYI That's consistent with what we do for react-native-codegen

yungsters · 2022-08-16T03:00:30Z

Hmm… I'm not sure why Danger is still failing.

Here are logs for the failing run: https://github.com/facebook/react-native/runs/7849986801?check_suite_focus=true

$ node ./node_modules/.bin/danger ci --use-github-checks --failOnErrors
Error:  Error: Failed to load parser '@typescript-eslint/parser' declared in '../.eslintrc.js » @react-native-community/eslint-config#overrides[1]': Cannot find module 'typescript'

But here is me testing Danger locally (and seeing it succeed):

$ cd react-native
$ yarn install
...
✨  Done in 24.79s.

$ ls node_modules/typescript/
AUTHORS.md                CopyrightNotice.txt       README.md                 ThirdPartyNoticeText.txt  lib/                      package.json              
CODE_OF_CONDUCT.md        LICENSE.txt               SECURITY.md               bin/                      loc/                      

$ cd bots
$ yarn install
...
✨  Done in 7.89s.

$ GITHUB_TOKEN=123456 node ./node_modules/.bin/danger pr https://github.com/facebook/react-native/pull/34401
Starting Danger PR on facebook/react-native#34401

Danger: ✓ found only warnings, not failing the build
## Warnings
:lock: package.json - <i>Changes were made to package.json. This will require a manual import by a Facebook employee.</i>
-
packages/event-emitter/src/EventEmitter.test.js line 127 – Unexpected alias 'result' for 'this'. (consistent-this)

Any ideas? 🫠

yungsters · 2022-08-16T03:03:24Z

Let me try moving the typescript dependency from packages/event-emitter/package.json to repo-config/package.json…

Edit: Hmm… nope. 😭

kelset · 2022-08-16T10:49:58Z

hey @yungsters I tried taking a quick look; there are a couple things that I think might be worth looking closer into (in no particular order):

I noticed that for this new package you are adding a .babelrc config, and I remembered that there was a whole thing about the different config files and stuff so I went back and found this doc: https://babeljs.io/docs/en/config-files (basically, .babelrc and babel.config.js have different scopes) - that actually made me realize/notice that not all packages have a dedicated .babelrc, nor that we have a root level babel.config.js (but that there's one in the template folder) (I wonder if that one is getting picked up and it's messing things up for the rest of the repo)
are the dependencies you are adding to the new package's package.json aligned with the rest of the repo? such as @babel/cli and jest; it looks like you are adding a ton of new packages to the yarn.lock and quite a few of those are babel dependencies - I think it'd be worth investigating what is causing this new wave of packages and attempt to deduplicate as much as possible
this would be the first ever package to also have TS typings right? My understanding is that there was some "magical mystical reason" that caused this repo to never accept typescript as a dependency (but I have nothing more than this as knowledge 🥲)

Aside from those more technical things, I also wanted to call out that it'd be best to also find a way to automate the release process for this package - since, aside core react-native all the other packages need to be manually published by some Meta engineers that owns access to the npm package 😅 finding a good solution for automatic releases would be a great blueprint to then "copy over" for the other packages :)

cortinico · 2022-08-16T16:28:45Z

Aside from those more technical things, I also wanted to call out that it'd be best to also find a way to automate the release process for this package - since, aside core react-native all the other packages need to be manually published by some Meta engineers that owns access to the npm package 😅 finding a good solution for automatic releases would be a great blueprint to then "copy over" for the other packages :)

That's essentially covered by the point 2. in this RFC comment: react-native-community/discussions-and-proposals#480 (comment)

kelset · 2022-08-16T16:44:54Z

oh yeah I know we are talking about it in the RFC; I just think it's likely that this PR will land before all that work is done 🤣

cortinico · 2022-08-17T10:19:11Z

oh yeah I know we are talking about it in the RFC; I just think it's likely that this PR will land before all that work is done 🤣

Totally. It's not worth to block this.

…facebook#34423) Summary: Changes the React Native base ESLint configuration to consume `react-native-community/eslint-config` as a [yarn workspace](https://classic.yarnpkg.com/lang/en/docs/workspaces/), so that any dependencies of `react-native-community/eslint-config` can also be resolved from the root directory of `react-native`. Previously, `~/.eslintrc.js` extended `react-native-community/eslint-config` using a relative file path. This is problematic because if any dependencies (notably, optional peer dependencies such as some of the TypeScript dependencies) are not already installed at the root directory of `react-native`, running ESLint could fail to resolve any required dependencies. In other words, there was an implicit dependency that `react-native/yarn.lock` would also contain any dependencies required by `react-native/packages/eslint-config-react-native-community/yarn.lock`. With this change, running `yarn` from the root directory of `react-native` will also install any dependencies of `react-native-community/eslint-config`, and it will also symlink `react-native/node_modules/react-native-community/eslint-config` to `../../packages/eslint-config-react-native-community` (meaning any local changes to the config will still be reflected during active development). ## Changelog [Internal] Pull Request resolved: facebook#34423 Test Plan: Successfully install dependencies and run ESLint. ``` $ cd react-native $ yarn $ yarn lint ``` Successfully run Danger on facebook#34401 (which would previously fail because `typescript-eslint/eslint-plugin` could not be resolved from the root directory of `react-native`): ``` $ cd react-native/bots $ yarn $ node ./node_modules/.bin/danger pr facebook#34401 ``` Reviewed By: NickGerleman Differential Revision: D38710285 Pulled By: yungsters fbshipit-source-id: a06ceea0884a90be60f6f5db9a5d42be52a951d5

Summary: currently, using TS, this is a valid import: `import { EventEmitter } from 'react-native';` However, looking at the [index file](https://github.com/facebook/react-native/blob/main/index.js) we can see that there is no such export. I first thought I'd add the EventEmitter export in order to get the `index.js` in line with the types, but it appears that the Event Emitter will become a separate package at some point #34401 so removing it from the types seems to be better for future. ## Changelog fix: remove unavailable EventEmitter TS export Pick one each for the category and type tags: [INTERNAL] [CHANGED] - remove unavailable EventEmitter TS export For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #36109 Test Plan: tested locally: using `import { EventEmitter } from 'react-native';` correctly gives `TS2305: Module '"react-native"' has no exported member 'EventEmitter'.` Reviewed By: javache, cortinico Differential Revision: D43155568 Pulled By: NickGerleman fbshipit-source-id: b9e8c3f4be9812637c8588d14a9ce4edf188ed36

github-actions · 2023-02-15T00:02:35Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2023-02-22T06:01:25Z

This PR was closed because it has been stalled for 7 days with no activity.

demedos · 2023-03-01T14:32:53Z

Any update on this?

yungsters · 2023-03-01T15:14:50Z

This is still something I plan to do, but I haven't had time to revisit it. I'll try to do this soon.

Summary: currently, using TS, this is a valid import: `import { EventEmitter } from 'react-native';` However, looking at the [index file](https://github.com/facebook/react-native/blob/main/index.js) we can see that there is no such export. I first thought I'd add the EventEmitter export in order to get the `index.js` in line with the types, but it appears that the Event Emitter will become a separate package at some point facebook#34401 so removing it from the types seems to be better for future. ## Changelog fix: remove unavailable EventEmitter TS export Pick one each for the category and type tags: [INTERNAL] [CHANGED] - remove unavailable EventEmitter TS export For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#36109 Test Plan: tested locally: using `import { EventEmitter } from 'react-native';` correctly gives `TS2305: Module '"react-native"' has no exported member 'EventEmitter'.` Reviewed By: javache, cortinico Differential Revision: D43155568 Pulled By: NickGerleman fbshipit-source-id: b9e8c3f4be9812637c8588d14a9ce4edf188ed36

yungsters requested a review from hramos as a code owner August 12, 2022 22:08

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Aug 12, 2022

yungsters requested review from rickhanlonii and cortinico August 12, 2022 22:08

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Aug 12, 2022

yungsters force-pushed the event-emitter branch 2 times, most recently from 6741133 to e62c227 Compare August 12, 2022 22:15

cortinico requested changes Aug 13, 2022

View reviewed changes

packages/event-emitter/package.json Outdated Show resolved Hide resolved

yungsters mentioned this pull request Aug 15, 2022

Configure @react-native-community/eslint-config as a yarn workspace #34423

Closed

yungsters force-pushed the event-emitter branch 3 times, most recently from e0efcc5 to cfbf7e7 Compare August 16, 2022 02:50

yungsters commented Aug 16, 2022

View reviewed changes

yungsters force-pushed the event-emitter branch 2 times, most recently from 2e2cf13 to a9bf973 Compare August 16, 2022 03:07

kelset mentioned this pull request Aug 16, 2022

RFC: React Native as a monorepo react-native-community/discussions-and-proposals#480

Merged

Separate Package for @react-native/event-emitter

2037697

yungsters force-pushed the event-emitter branch from a9bf973 to 2037697 Compare August 18, 2022 20:39

yungsters mentioned this pull request Oct 14, 2022

fix: Eslint on Danger not matching project config #34980

Closed

vonovak mentioned this pull request Feb 9, 2023

fix: remove unavailable EventEmitter TS export #36109

Closed

github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 15, 2023

github-actions bot closed this Feb 22, 2023

yungsters deleted the event-emitter branch July 11, 2023 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Package for `@react-native/event-emitter` #34401

Separate Package for `@react-native/event-emitter` #34401

yungsters commented Aug 12, 2022 •

edited

Loading

github-actions bot commented Aug 12, 2022 •

edited

Loading

analysis-bot commented Aug 12, 2022 •

edited

Loading

analysis-bot commented Aug 12, 2022 •

edited

Loading

yungsters commented Aug 12, 2022

yungsters commented Aug 15, 2022

yungsters Aug 16, 2022

cortinico Aug 16, 2022

yungsters commented Aug 16, 2022 •

edited

Loading

yungsters commented Aug 16, 2022 •

edited

Loading

kelset commented Aug 16, 2022

cortinico commented Aug 16, 2022 •

edited

Loading

kelset commented Aug 16, 2022

cortinico commented Aug 17, 2022

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 22, 2023

demedos commented Mar 1, 2023

yungsters commented Mar 1, 2023

Separate Package for @react-native/event-emitter #34401

Separate Package for @react-native/event-emitter #34401

Conversation

yungsters commented Aug 12, 2022 • edited Loading

Summary

Changelog

Test Plan

github-actions bot commented Aug 12, 2022 • edited Loading

Log

analysis-bot commented Aug 12, 2022 • edited Loading

analysis-bot commented Aug 12, 2022 • edited Loading

yungsters commented Aug 12, 2022

yungsters commented Aug 15, 2022

yungsters Aug 16, 2022

Choose a reason for hiding this comment

cortinico Aug 16, 2022

Choose a reason for hiding this comment

yungsters commented Aug 16, 2022 • edited Loading

yungsters commented Aug 16, 2022 • edited Loading

kelset commented Aug 16, 2022

cortinico commented Aug 16, 2022 • edited Loading

kelset commented Aug 16, 2022

cortinico commented Aug 17, 2022

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 22, 2023

demedos commented Mar 1, 2023

yungsters commented Mar 1, 2023

Separate Package for `@react-native/event-emitter` #34401

Separate Package for `@react-native/event-emitter` #34401

yungsters commented Aug 12, 2022 •

edited

Loading

github-actions bot commented Aug 12, 2022 •

edited

Loading

analysis-bot commented Aug 12, 2022 •

edited

Loading

analysis-bot commented Aug 12, 2022 •

edited

Loading

yungsters commented Aug 16, 2022 •

edited

Loading

yungsters commented Aug 16, 2022 •

edited

Loading

cortinico commented Aug 16, 2022 •

edited

Loading