Skip to content

react-aria/i18n circular dependency #3628

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

Closed
qwhex opened this issue Oct 11, 2022 · 20 comments · Fixed by #5087
Closed

react-aria/i18n circular dependency #3628

qwhex opened this issue Oct 11, 2022 · 20 comments · Fixed by #5087

Comments

@qwhex
Copy link

qwhex commented Oct 11, 2022

🐛 Bug Report

We use react-aria extensively in our project, and when we upgraded to react-aria 3.19.0, a new warning appeared regarding the circular dependency between react-aria/i18n/dist/module.js and react-aria/i18n/dist/useMessageFormatter.module.js.

Upgrading to 3.20.0 didn't solve the issue.

We couldn't find any mention of this issue here, and also couldn't find the culprit on our end, so we decided to raise it.

If you think it's a problem on our end, we'd be happy to receive any pointers on what to look into.

🤔 Expected Behavior

No circular dependencies present.

😯 Current Behavior

A warning is shown about the circular dependency in react-aria/i18n when we're building our package:

[...]

$ yarn build
yarn run v1.22.19
$ rollup -c

src/index.ts → dist/index.cjs.js, dist/index.esm.js...
(!) Circular dependency
node_modules/@react-aria/i18n/dist/module.js -> node_modules/@react-aria/i18n/dist/useMessageFormatter.module.js -> node_modules/@react-aria/i18n/dist/module.js

[...]

🌍 Your Environment

Software Version(s)
react-aria 3.19.0 - 3.20.0
react 17.0.2
typescript 4.7.4
nodejs 18.9.0
@snowystinger
Copy link
Member

snowystinger commented Oct 11, 2022

Hey, thanks for the issue. We haven't seen this one yet. I'm looking through those packages on our end, but I'm not seeing anything indicating we have a circular dependency. Would you be willing to look in node_modules and see what the actual circle is? (what from each file is depending on the other)

@snowystinger
Copy link
Member

Another thing you can do is try to reproduce the error in a more minimal setting
for instance, something like codesandbox https://codesandbox.io/s/goofy-kapitsa-gth27d?file=/src/App.js

I don't know if I'd need to pull down the codesandbox locally to see the error, but the app is running, so I have fairly high hopes that this is showing a no-error case.

@qwhex
Copy link
Author

qwhex commented Oct 12, 2022

Hello! We appreciate the quick reply.

I looked into the file contents and I found this:

module.js has the following exports:

export * from './real-module.js';
export * from './useMessageFormatter.module.js';

and useMessageFormatter.module.js has this import:

import { useLocale } from '../';

by changing the line to the one below, the warning disappears:

import { useLocale } from './real-main.js';

@snowystinger
Copy link
Member

Thanks for doing that sluething. I have a hunch that if we change

import { useLocale } from '../';

to this

import {useLocale} from './context';

then that may fix the issue

good news though, it's just a warning and useMessageFormatter is deprecated, so you should be able to avoid any issues with it

@snowystinger
Copy link
Member

Created a PR with the change, I'm seeing a different structure now in our produced dist. I don't know if you'd be willing to build us locally and check if gets rid of the warning.

An easy way to test us built locally is to:

  1. pull down the branch you want
  2. run ./scripts/verdaccio.sh (this will start a local npm proxy registry, and build and publish the branch code to it)
  3. go to a different terminal window and go to your project
  4. delete your node_modules
  5. set your registry temporarily to the proxy server's url
  6. run install and run whatever you need to in order to verify
  7. done, you can reset your registry and stop the verdaccio server

@reidbarber reidbarber added the waiting Waiting on Issue Author label Oct 19, 2022
@snowystinger
Copy link
Member

This should be fixed by #4038
Closing for now, if you find it to still be an issue, it can be reopened

@ohazda
Copy link

ohazda commented Jun 14, 2023

@snowystinger it seems like an issue exists still exists. Would be great to update this line https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/i18n/src/useMessageFormatter.ts#L15 to import from context as per your suggestion. Even though the useMessageFormatter is deprecated there is no documentation about alternative function. Plus it requires some extra work to migrate to it.

@brandonpittman
Copy link
Contributor

brandonpittman commented Jun 23, 2023

@snowystinger

I'm trying to export a RAC Button in my design system package and this circular dependency is breaking the consumer's build—or maybe more correctly, the consumer app build never finishes.

// library build
(!) Circular dependency
node_modules/@react-aria/i18n/dist/import.mjs -> node_modules/@react-aria/i18n/dist/useMessageFormatter.module.mjs -> node_modules/@react-aria/i18n/dist/import.mjs

@snowystinger snowystinger removed the waiting Waiting on Issue Author label Jun 23, 2023
@snowystinger
Copy link
Member

What build system are you using? I'll reopen in a little bit.

@brandonpittman
Copy link
Contributor

brandonpittman commented Jun 23, 2023

Building with Rollup and then consuming in a Remix app. Styles are done with Vanilla Extract. Before importing a React Aria Button, builds on both sides are fine. After importing, get a circular dependency warning in Rollup and the Remix app never completes building.

@snowystinger snowystinger reopened this Jun 23, 2023
@brandonpittman
Copy link
Contributor

brandonpittman commented Jun 27, 2023

@snowystinger I tried looking for an example of packaging up custom components built on top of React Aria (Components) with Rollup but couldn't find one. Could you point me to an example? Maybe I could see if I'm doing something wrong.

@snowystinger
Copy link
Member

I don't know any Rollup builds, unfortunately. We're also busy right now with some other priorities, so if this is a priority for you, then you'll either need to submit a fix (in which case you could use patch-package or something to apply it to your build) or see if there is a way to tell rollup to ignore this.

I'm a little surprised that the new RAC stuff is using the deprecated useMessageFormatter though. Will look into this a bit more when I get some time.

@brandonpittman
Copy link
Contributor

@snowystinger My Rollup config seems fine. I just removed all references to useMessageFormatter with patch-package and my components built successfully and my Remix app started correctly. It's really just this circular dependency causing problems.

@ashleyryan
Copy link

ashleyryan commented Sep 12, 2023

We're also hearing from one of our teams of this circular dependency issue. The only thing we directly reference from react-aria/i18n is useFilter for combobox filtering.

Working on getting more details about their build setup, it's in an NX monorepo. They're using a version of our package that uses react-aria/i18n 3.7.1 and I'm asking them to try with the latest of ours, which is on 3.8.1

@snowystinger
Copy link
Member

snowystinger commented Sep 15, 2023

Thanks for letting us know. While we can see the circular dependency warning, it's just a warning. From our testing, with esbuild though, which remix uses, it doesn't appear to cause an issue.
You can see the output of rollup here, just run the build script.
https://stackblitz.com/edit/stackblitz-starters-qjzgfv?file=package.json

We think we can get rid of the circular dependency anyways with a post build script. We'd need to make the corresponding change in each output file
https://unpkg.com/browse/@react-aria/[email protected]/dist/useMessageFormatter.module.mjs needed to change import { useLocale } from '@react-aria/i18n'; -> import { useLocale } from './real-module.mjs';

It'd be good to get an actual reproduction of the failure though so we can verify that it worked completely.

@snowystinger
Copy link
Member

I've verified that the fix works for rollup with patch-package in the example repo I shared, the warning no longer appears. If @brandonpittman or @ashleyryan could also check with patch-package, that'd be a huge help.
Once it's merged, we could also try with the nightlies. I'll post here when that happens.

@snowystinger
Copy link
Member

Branch has been merged, it'll be in tonight's nightly release

@marcod1419
Copy link

marcod1419 commented Mar 1, 2024

Seeing this pop up again, also with rollup. Same issue too, on the latest react-aria-components (v1.1.1) package.

The imports I'm using are:

import { DateField as AriaDateField, DateInput, DateSegment } from 'react-aria-components'
import { I18nProvider, useId } from 'react-aria'
import { parseDate } from '@internationalized/date'

And the warning I get is:

(!) Circular dependency
node_modules/@react-aria/i18n/dist/import.mjs -> node_modules/@react-aria/i18n/dist/useMessageFormatter.module.mjs -> node_modules/@react-aria/i18n/dist/import.mjs

Filtering the warning out using my rollup config seems to work fine for now, but thought I'd let you all know!

@snowystinger
Copy link
Member

Interesting, thanks for letting us know. The build hasn't changed since we introduced the fix for this. Has it been happening the whole time? or did it just start again with a particular release?

@snowystinger
Copy link
Member

For those still seeing an issue, this may be addressed with #6064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants