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

REF-1050/Investigate Windows Hot reload #2106

Merged
merged 7 commits into from Nov 8, 2023

Conversation

ElijahAhianyo
Copy link
Collaborator

@ElijahAhianyo ElijahAhianyo commented Nov 1, 2023

This PR fixes a peer dependency resolution error by the postcss lib causing a relock of the package-lock.json file.

@ElijahAhianyo ElijahAhianyo changed the title REF-1050/Investigate Windows Hot reload [WIP]REF-1050/Investigate Windows Hot reload Nov 1, 2023
@ElijahAhianyo ElijahAhianyo force-pushed the REF-1050/investigate-windows-reload branch from 35c0abe to 3be5099 Compare November 2, 2023 12:42
@ElijahAhianyo ElijahAhianyo changed the title [WIP]REF-1050/Investigate Windows Hot reload REF-1050/Investigate Windows Hot reload Nov 2, 2023
@ElijahAhianyo ElijahAhianyo marked this pull request as ready for review November 2, 2023 13:07
masenf
masenf previously approved these changes Nov 2, 2023
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

cool!

@masenf
Copy link
Collaborator

masenf commented Nov 2, 2023

It doesn't seem significantly faster on my windows VM. Still installs tailwind every time...

@masenf
Copy link
Collaborator

masenf commented Nov 2, 2023

also seems like this turns up the log level of npm by default? my terminal is displaying a lot more output now

@ElijahAhianyo
Copy link
Collaborator Author

It doesn't seem significantly faster on my windows VM. Still installs tailwind every time...

I see, when you look at the npm logs for tailwind, does the cache get revalidated every time?

@masenf
Copy link
Collaborator

masenf commented Nov 2, 2023

does the cache get revalidated every time

not sure how to tell...

@ElijahAhianyo
Copy link
Collaborator Author

also seems like this turns up the log level of npm by default? my terminal is displaying a lot more output now

I added this as a result of this issue(#2115) where the installation fails but theres not much details on why. Probably there's a better way to handle this, Maybe move that a log level up(Error instead of debug)?

@ElijahAhianyo
Copy link
Collaborator Author

ElijahAhianyo commented Nov 2, 2023

does the cache get revalidated every time

not sure how to tell...

usually you'd see this in the logs when its revalidated:

Debug: Running command: ['/opt/homebrew/bin/npm', 'add', '-d', 'tailwindcss@3.3.2', '@tailwindcss/typography']
Debug: Installing tailwind
⠙ Installing tailwindnpm info using npm@9.8.0
npm info using node@v18.17.0
npm http fetch GET 200 https://registry.npmjs.org/@tailwindcss%2ftypography 2824ms (cache revalidated)
npm http fetch GET 200 https://registry.npmjs.org/tailwindcss 1195ms (cache revalidated)
npm http fetch POST 200 https://registry.npmjs.org/-/npm/v1/security/advisories/bulk 489ms

Also just to get a sense of how little of a difference it makes for you, what are the time differences(or numbers) when you run with the npm_prefer_offline vs when you dont?
Also, running right after reflex init may not hit the cache and hence revalidate. Expected result on reload should show a (cache stale)

@masenf
Copy link
Collaborator

masenf commented Nov 2, 2023

When i run with npm_prefer_offline, it takes about 12 seconds from when i hit save to when the npm output stops scrolling.

When i run without npm_prefer_offline, it's about the same, 13 seconds from when i hit save.

As for the tailwind thing, i think my output is getting screwed up on windows, because i see this when it mentions tailwind

npm info ok
⠋ Installing tailwindnpm info using npm@9.6.7
npm info using node@v18.17.0
npm http fetch GET 200 https://registry.npmjs.org/tailwindcss 673ms (cache updated)
⠋ Installing tailwind[##################] / reify:@next/swc-darwin-arm64: timing reifyNode:node_modules/@next/swc-darwinnpm http fetch POST 200 https://registry.npmjs.org/-/npm/v1/security/advisories/bulk 130ms
⠙ Installing tailwind[##################] \ reify:@next/swc-darwin-arm64: http fetch POST 200 https://registry.npmjs.org

@Alek99
Copy link
Contributor

Alek99 commented Nov 6, 2023

I see this is approved, but may have some issues. What is the best way to go about testing this

@ElijahAhianyo
Copy link
Collaborator Author

ElijahAhianyo commented Nov 6, 2023

Moving the performance tests to a sub PR(#2136) as I need to do a bit more investigation on why this behaves differently on other windows

@picklelo picklelo merged commit 96eca4f into main Nov 8, 2023
37 checks passed
@masenf masenf deleted the REF-1050/investigate-windows-reload branch December 6, 2023 22:19
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.

None yet

4 participants