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

Fresnel 2 #19

Merged
merged 22 commits into from
Aug 23, 2020
Merged

Fresnel 2 #19

merged 22 commits into from
Aug 23, 2020

Conversation

cmaycumber
Copy link
Contributor

@cmaycumber cmaycumber commented Jul 30, 2020

Description

Changed configuration of example project by removing the tsconfig.json, adding tweaks to the webpack config and babel config to get the example app working across different apps. Added react and react-native as peer dependencies in dripsy.

Working on #14

Devices tested on example app w/o SSR option on DripsyProvider

Example

  • Web
  • IOS
  • Android

NextJs Example

  • Web
  • IOS
  • Android

Copy link
Owner

@nandorojo nandorojo left a comment

Choose a reason for hiding this comment

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

The SSR option for the dripsy provider will probably get deprecated to keep all web behavior consistent, but good to keep it for the time being.

@nandorojo
Copy link
Owner

Could we leave the src/components file in the other PR? It seems that there are still some unresolved comments there.

@@ -1,16 +1,24 @@
import { createThemedComponent } from '../css/create-themed-component'
import {
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind if we disregard this file for this PR, and instead merge from #15 once the issues mentioned there are resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes will do!

@cmaycumber
Copy link
Contributor Author

I'll leave out the components. Those must have snuck in there my bad.

@nandorojo
Copy link
Owner

Cool no worries

@nandorojo
Copy link
Owner

For the web test, does this include SSR (i.e. the next example?)

@cmaycumber
Copy link
Contributor Author

No, I didn't test the next-example project. I can run the test for that one too.

@nandorojo
Copy link
Owner

Awesome, thanks for taking it on. I've been a bit swamped this week finishing another project, but I should be done with it by EOW and will have more bandwidth to close out the remaining issues.

@nandorojo
Copy link
Owner

Re: the next example, I remember having problems with it, and the way I fixed it was removing react from peer deps, as I see you've added here. This was my workflow:

cd dripsy
yarn link
cd next-example
yarn link dripsy

yarn next dev

For some reason, yarn link wasn't happy when I had react in the peer deps.

@cmaycumber
Copy link
Contributor Author

@nandorojo I'm running into similar problems getting the next example working. I got a configuration that gets all the way to build a build and then runs into an invariant violation from multiple versions of react. I'll try rollback some of my changes and give your method a shot.

@nandorojo
Copy link
Owner

nandorojo commented Aug 12, 2020

Yeah, the multiple versions of React was solved for me by getting rid of the react peer dependency. But it sounds like this gets in the way of your expo solution.

@cmaycumber
Copy link
Contributor Author

Got it working by wrapping the withExpo plugin around the withTM plugin. I'm still running into the same issue I had with the gatsby site I was working on, it doesn't look like fresnel is removing the additional dom nodes with match media but instead keeping them all. Here's a screenshot:

Screen Shot 2020-08-12 at 4 51 06 PM

Not sure if this was intended or not but I thought that match-media was supposed to do some cleanup.

@nandorojo
Copy link
Owner

nandorojo commented Aug 12, 2020

I see. It looks like the fresnel nodes are still there, but they are cleaning up their children, correct?

Seems like only the at-0 is rendered.

@cmaycumber
Copy link
Contributor Author

They are cleaning up their children. So fresnel should cleanup only children elements but not the parent containers?

@nandorojo
Copy link
Owner

So fresnel should cleanup only children elements but not the parent containers?

Good question...I wonder what their thinking is behind that. Maybe we could make an issue on their repo? They were pretty responsive when I asked questions there before.

@cmaycumber
Copy link
Contributor Author

Good question...I wonder what their thinking is behind that. Maybe we could make an issue on their repo? They were pretty responsive when I asked questions there before.

That's probably the move maybe they can shed some light on it.

@cmaycumber
Copy link
Contributor Author

Opened an issue here: artsy/fresnel#147

@nandorojo
Copy link
Owner

Curious to see what they say.

@nandorojo
Copy link
Owner

I see that you got this working with Next.js. I'm trying to get this tested on my end. Are you using yarn link, or just using yarn pack and installing the .tgz file in next-example?

@nandorojo
Copy link
Owner

Next example seems to be working well for me on web!

I had to yarn pack again since there were some permissions issues, and then it worked. I wonder if there is any other way to get this example to work down the line so it's more easily symlinked...probably an issue for later.

However, if I expo start --ios -c in /next-example, I see this error:

Screen Shot 2020-08-16 at 7 31 41 PM

I think it's because expo doesn't support monorepos out of the box. When I copied your custom config from /example into /next-example, this error was fixed (babel, metro and webpack config files.) I'm not very familiar with babel, metro or webpack, so I'm not sure if all three files were needed in the /next-example or not, but adding them did make it work!

Could you add the necessary config files from your /example to /next-example, and confirm that /next-example runs on iOS / Android for you after that? Assuming it does, this looks good to merge for me. Great work.

(I had no issues running the /example folder here, worked everywhere for me.)

@cmaycumber
Copy link
Contributor Author

Sweet good to hear! I'll try to get this working so that these can be merged in. Once we get some feedback from the fresnel guys we should be gtg.

@nandorojo
Copy link
Owner

nandorojo commented Aug 18, 2020

Great. Assuming the fresnel lib can clean up the extraneous parent nodes, I don't see how using it could be less performant than using React Native's Dimensions API.

@cmaycumber
Copy link
Contributor Author

Yeah, should be sweet. It looked like overall that it was working pretty well.

@nandorojo
Copy link
Owner

Could this help for the Next.js symlinks? martpie/next-transpile-modules#9 (comment)

@cmaycumber
Copy link
Contributor Author

I'd be interested in trying to use yarn workspaces with it. I've had success in the past with yarn workspaces + expo-yarn-workspaces. Not sure how well it plays with next, but I'm down to try because right now it's painful getting them to work nicely together.

@nandorojo
Copy link
Owner

Yeah I was thinking the same thing. I really like expo + next.js, and it would be great to figure out how they mesh together with yarn workspaces.

@cmaycumber
Copy link
Contributor Author

Same I think that would be cool. I'm running it with a gatsby project rn and it works well I don't see why that wouldn't translate to next. I don't mind trying to get the repo working with yarn workspaces after we get some of this other stuff done.

@nandorojo
Copy link
Owner

This seems to be working all around for me. Confirming that it's good to merge?

@nandorojo
Copy link
Owner

Same I think that would be cool. I'm running it with a gatsby project rn and it works well I don't see why that wouldn't translate to next. I don't mind trying to get the repo working with yarn workspaces after we get some of this other stuff done.

Agreed. I think it would be great to get a workspace working for this, since local testing with yarn pack is a bit of a pain.

@cmaycumber
Copy link
Contributor Author

It should be good to merge. I'll try to get a yarn workspace version of the repo working later today.

@nandorojo
Copy link
Owner

nandorojo commented Aug 23, 2020

The only problem I'm running into in the example is #26, which I solved with this. But it's not a problem specific to this PR, so I'll leave that change out.

@nandorojo nandorojo merged commit 70f7b75 into nandorojo:fresnel-2 Aug 23, 2020
@nandorojo
Copy link
Owner

nandorojo commented Sep 19, 2020

The resizing really isn't that bad when it's published in production. The lag is far more noticeable in dev mode.

Final point. The one annoying thing about using this branch is that you often have to use the webContainerSx prop when making flex boxes stretch. It’s not that bad, but it will require some documenting when people inevitably face it.

Example:

View (flex direction row)
  - View (flex: [0, 1])

The child View has an array style, which means it will be wrapped with the Fresnel div. However, it won't respect the flex: 1 relative to its parent, since the fresnel div wraps it.

As a result, you have to do something like:

<View 
  sx={{ flex: [0, 1] }}
  webContainerSx={{ flex: [0, 1] }}
/>

I tried spreading the sx flex value onto webContainerSx, but it didn't make any sense, since the actual View became flexible relative to the fresnel one. It will just require noticing the behavior, and adjusting your webContainerSx for your use-case.

There will probably just need to be a section in the docs about this, especially describing the behavior with flex. I don't think I've really run into this with any styles other than flex.

All that said, I do feel pretty comfortable merging this. I don't have much quantitative data on performance, so I'll try to use the profilers before doing so, but it seems like it's all pretty quick to me, even though I'm generating an insane number of DOM nodes.

(Down the line, it could be worth figuring out a way to achieve what Fresnel does with Fragment. But since we need a div with className on our first render, this probably isn't feasible. This might not be needed at all though, based on my current experience with Dripsy speed.)

@nandorojo nandorojo mentioned this pull request Sep 23, 2020
3 tasks
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.

2 participants