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

WIP: Importing from shared dir #370

Closed
wants to merge 11 commits into from
Closed

Conversation

pcowgill
Copy link
Member

@pcowgill pcowgill commented May 8, 2019

Issue link

#336

Auto-close the issue?

Closes #336

Types of changes

Technical debt (a code change that doesn't fix a bug or add a feature but makes something clearer for devs)

Notes

This seems like it should be working based on comments I found when googling like this one facebook/react-native#12241 (comment), but it isn't working yet.

@pcowgill pcowgill changed the title WIP: importing from shared dir WIP: Importing from shared dir May 8, 2019
@pcowgill
Copy link
Member Author

pcowgill commented May 8, 2019

This might be related expo/expo@1c24b4e

@marcelomorgado
Copy link
Contributor

Do you want that I take a look on this?

@pcowgill
Copy link
Member Author

pcowgill commented May 8, 2019

Do you want that I take a look on this?

@marcelomorgado Sure, that would be great! Thanks!

@pcowgill
Copy link
Member Author

pcowgill commented May 8, 2019

@marcelomorgado This is the clearest reference repo I found, where there's an expo app that uses a shared component https://github.com/MPiwowarski/react-native-adjacent-imports

@pcowgill
Copy link
Member Author

pcowgill commented May 8, 2019

@marcelomorgado This is the clearest reference repo I found, where there's an expo app that uses a shared component https://github.com/MPiwowarski/react-native-adjacent-imports

I found it here facebook/react-native#12241 (comment)

@@ -1,6 +1,14 @@
const path = require("path");

module.exports = {
// Both lines extracted from: https://medium.com/react-native-training/sharing-code-between-react-web-and-react-native-applications-7f451af26378
// Tech-debt: Use same var for the shared dir (Why using resolve instead of join?)
Copy link
Member Author

Choose a reason for hiding this comment

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

Having looked at the path docs, either resolve or join is a perfectly sensible approach to achieve the same thing

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by Use same var for the shared dir?

Copy link
Contributor

Choose a reason for hiding this comment

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

You've created var shared = path.join(__dirname, "../shared");. I've introduced watchFolders: [path.resolve(__dirname, "../shared")],, I've meant to used shared variable one watchFolders too if is possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, understood!

@marcelomorgado
Copy link
Contributor

Now, we're getting this error (from expo-cli):
Unable to resolve "@babel/runtime/helpers/interopRequireDefault" from "../shared/components/presentational/Button.js"

@pcowgill
Copy link
Member Author

pcowgill commented May 9, 2019

Unable to resolve "@babel/runtime/helpers/interopRequireDefault"

@marcelomorgado Did you do npm i in the shared dir before running the decentraland app when you saw this?

@pcowgill
Copy link
Member Author

pcowgill commented May 9, 2019

@marcelomorgado I'm getting this naming collision for react-native which is a dep of both the shared dir and the decentraland dir, but that's to be expected.

[16:47:11] (node:64828) UnhandledPromiseRejectionWarning: Error: jest-haste-map: @providesModule naming collision:
[16:47:11]   Duplicate module name: react-native
[16:47:11]   Paths: /Users/paulcowgill/Code/tasitlabs/tasit/shared/node_modules/react-native/package.json collides with /Users/paulcowgill/Code/tasitlabs/tasit/decentraland/node_modules/react-native/package.json

I know I saw someone handling that in one of the threads I looked at while researching this. I'll look for that.

Side note: If there are enough shared deps we might want lerna in this repo too #374

@pcowgill
Copy link
Member Author

pcowgill commented May 9, 2019

I'm thinking about splitting the deps up like this. At this point almost all of hte deps can probably go in the shared dir unless we're really sure it's decentraland-specific or a now-debatable choice we might move away from for tech debt reasons like redux.

Screenshot 2019-05-09 17 04 41

@pcowgill
Copy link
Member Author

pcowgill commented May 9, 2019

Or maybe I spoke too soon:

[17:06:44] Error: Can't find react-native in package.json dependencies

[17:06:44] Error: React Native is not installed. Please run `npm install` in your project directory.

[17:06:44] Couldn't start project. Please fix the errors and restart the project.

[17:06:44] Set EXPO_DEBUG=true in your env to view the stack trace.

I'll keep researching how others have done this

@pcowgill
Copy link
Member Author

pcowgill commented May 9, 2019

This was the related comment I remembered expo/create-react-native-app#232 (comment)

@pcowgill
Copy link
Member Author

pcowgill commented May 9, 2019

metro changed their config format, but this suggestion seems like it will work:

facebook/metro#7 (comment)

@pcowgill
Copy link
Member Author

metro changed their config format, but this suggestion seems like it will work:

facebook/metro#7 (comment)

I think this is the right direction to be headed in, but it hasn't solved the issue of complaining about duplicated react and react-native deps just yet.

@pcowgill
Copy link
Member Author

Unable to resolve "@babel/runtime/helpers/interopRequireDefault"

@marcelomorgado Did you do npm i in the shared dir before running the decentraland app when you saw this?

@marcelomorgado I'm not seeing the babel issue - does running npm i address it for you?

@marcelomorgado
Copy link
Contributor

Unable to resolve "@babel/runtime/helpers/interopRequireDefault"

@marcelomorgado Did you do npm i in the shared dir before running the decentraland app when you saw this?

@marcelomorgado I'm not seeing the babel issue - does running npm i address it for you?

I'm getting the same error:

[16:09:04] Error: Can't find react-native in package.json dependencies

[16:09:04] Error: React Native is not installed. Please run `npm install` in your project directory.

[16:09:04] Couldn't start project. Please fix the errors and restart the project.

[16:09:04] Set EXPO_DEBUG=true in your env to view the stack trace.

@pcowgill
Copy link
Member Author

Unable to resolve "@babel/runtime/helpers/interopRequireDefault"

@marcelomorgado Did you do npm i in the shared dir before running the decentraland app when you saw this?

@marcelomorgado I'm not seeing the babel issue - does running npm i address it for you?

I'm getting the same error:

[16:09:04] Error: Can't find react-native in package.json dependencies

[16:09:04] Error: React Native is not installed. Please run `npm install` in your project directory.

[16:09:04] Couldn't start project. Please fix the errors and restart the project.

[16:09:04] Set EXPO_DEBUG=true in your env to view the stack trace.

If I include react-native as a dep in both decentraland and shared, that issue goes away, but then I get the duplicated provides module error. Which I thought extraNodeModules would address

@pcowgill pcowgill closed this Nov 13, 2019
@pcowgill pcowgill deleted the feature/shared-components branch June 9, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract My profile screen components to shared dir
2 participants