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

Use commonjs imports in relay-runtime #2367

Closed
wants to merge 2 commits into from

Conversation

robrichard
Copy link
Contributor

@robrichard robrichard commented Mar 13, 2018

Doing the same that was done in 97f24f8 for react-relay in relay-runtime.

This is part of the first step in #1590 (comment).

I left the @providesModule in the files in relay-runtime/util/*.js and a few others that used directly in the other packages. I can address that in the next PR by exporting them in RelayRuntime.js. I didn't want this one to get bigger than it already is.

@alloy
Copy link
Contributor

alloy commented Mar 14, 2018

Nice 👌

I think it would make sense to add a lint rule, such as this one by @janicduplessis, to this repo. There’s probably a bunch of changes needed of all open PRs and it could be hard to remember for FB employees to not use Haste imports as it will just work for them.

alloy referenced this pull request Mar 15, 2018
Reviewed By: dwwoelfel

Differential Revision: D7209118

fbshipit-source-id: 109c291e499de4ad4c0434db6f4e31db175993d6
@kassens
Copy link
Member

kassens commented Mar 15, 2018

Sorry, this is blocked by deep reliance on the @providesModule names to import flow types and private modules all over the place. I hope I can make some progress on this at some point, but it's not the highest priority :(

When we have this, I think the open source will be cleaner, but it's a mess to change this.

@robrichard
Copy link
Contributor Author

robrichard commented Mar 15, 2018

@kassens is there any point in leaving the @providesModule but still updating the requires to relative paths?

@kassens
Copy link
Member

kassens commented Mar 23, 2018

In one of our environments we don't support a mix of both. The @providesModule version is the one "native" to most FB code and commonjs somewhat of a hack. I'll see if I can make slow progress on this as it would make a bunch of things easier like proper flow type exports.

@alloy
Copy link
Contributor

alloy commented Mar 23, 2018

Very naive thought, but would it be feasible to have a script for that one environment that changes commonjs imports to haste ones and moves all the files back into a flat structure?

@robrichard
Copy link
Contributor Author

@kassens would it be possible to update relay to use ES6 modules while still maintaining compatibility with the FB haste system?

If it's possible, I'd like to open PR to do that to get closer to shipping optimized flat bundles (#1590 (comment)).

@robrichard robrichard closed this Mar 26, 2018
@robrichard robrichard reopened this Mar 26, 2018
@kassens
Copy link
Member

kassens commented Apr 27, 2018

Hey, I think it doesn't make sense to have this as a PR as the hardest part is figuring out the internal cleanup which probably needs to happen somewhat incrementally to keep the changes sane. (A bunch of internal references need to be updates when a module is moved).

@kassens kassens closed this Apr 27, 2018
@robrichard robrichard deleted the dehaste branch November 3, 2020 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants