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

fix: #100 #102 #107

Merged
merged 2 commits into from
Sep 3, 2023
Merged

fix: #100 #102 #107

merged 2 commits into from
Sep 3, 2023

Conversation

henilp105
Copy link
Collaborator

@henilp105 henilp105 commented Sep 3, 2023

PR Reopened from the main repo.

Fix: #100

Towards: #102

resets the code area for each refresh.
Checks if there are any other parameters than the defined, if there are shows the error.

Thanks,
Henil

CC: @certik @Shaikh-Ubaid

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Your site is deployed at https://lfortran.github.io/pull_request_preview/lfortran/107

pages/index.js Outdated Show resolved Hide resolved
Co-authored-by: Ondřej Čertík <[email protected]>
@certik
Copy link
Contributor

certik commented Sep 3, 2023

I updated the gist, and the updated gist does not show, it shows the old one. So I would say this PR does not fix #102.

It might be a github issue that it takes time to serve the new gist, or perhaps it is cached somewhere else.

It shows it now. Maybe it just takes time to update.

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Your site is deployed at https://lfortran.github.io/pull_request_preview/lfortran/107

@certik
Copy link
Contributor

certik commented Sep 3, 2023

I updated the gist https://gist.github.com/certik/7e2652943bbff7f0d0963dd4fcf1813a at 7:56 to "OK 3", but this link: https://lfortran.github.io/pull_request_preview/lfortran/107?gist=certik/7e2652943bbff7f0d0963dd4fcf1813a still shows "OK 2" at 8:01.

I tested on my phone, there it says "OK 3" but in by browser it still says "OK 2".

I tried a private window in Firefox, that shows "OK 3", but the main window still shows "OK 2". I also tried hard refresh with "Command+Shift+R", still "OK 2".

@Shaikh-Ubaid any idea what might be causing this caching bug?

And finally, at 8:04 it shows the updated content in my main window also.

@henilp105
Copy link
Collaborator Author

It seems they have some cdn infrastructure, thus caching is taking place at cdn , now it shows the updated content.

@certik
Copy link
Contributor

certik commented Sep 3, 2023

@henilp105 if they have a CDN, shouldn't that be cached for both a private window and the main window in the same way? It's only cached if I loaded the old value before it seems.

I changed to "OK 4" but never loaded in the app. Then I changed to "OK 5". In private window it updates immediately. In the main window it is currently stuck at "OK 3" still.

Let's keep #102 open for now. Anybody who updates a gist will hit this issue. It does seem to resolve over time, but if we can be sure that it will always fix itself in 10 minutes, then we can just document it.

@henilp105
Copy link
Collaborator Author

Yes @certik , the other cause might be the react variables getting cached on the client side.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this looks good, thanks!

It works.

@certik certik merged commit fe13d54 into main Sep 3, 2023
1 check passed
@certik certik deleted the fix-links branch September 3, 2023 14:11
@certik
Copy link
Contributor

certik commented Sep 3, 2023

Yes @certik , the other cause might be the react variables getting cached on the client side.

Yes, I wonder if this could be the case.

@@ -51,6 +51,7 @@ export default function Home() {
const myHeight = ((!isMobile) ? "calc(100vh - 170px)" : "calc(50vh - 85px)");

useEffect(() => {
setSourceCode("");
Copy link
Member

Choose a reason for hiding this comment

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

By default the source code is already "" (an empty string). @henilp105 please, could you share the benefit of setting it again here?

resets the code area for each refresh.

I think the code area will already be an empty string "" for every refresh. I am confused about how this change helps us. I would be grateful if you could enlighten me, Henil.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes , this would only be helpful on the refresh of the page when there is content already in SourceCode as use effect will be called due to no conditions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes , this would only be helpful on the refresh of the page when there is content already in SourceCode

I mean that sourceCode already has an initial value of "" at the following place:

const [sourceCode, setSourceCode] = useState("");

Thus, I think on refresh of the page there will not any be any content in sourceCode (because on refresh, sourceCode would have its default value which is an empty string). Thus it seems explicity setting setSourceCode("") before the fetchData() is not needed. It seems to be redundant. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is redundant, then we can remove it. It's just a one line change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I think we should remove it.

@Shaikh-Ubaid
Copy link
Member

any idea what might be causing this caching bug?

I believe it is most likely a react issue. I am hoping there is a way to set no-caching for the fetch request. We might need to explore.

@Shaikh-Ubaid
Copy link
Member

I am hoping there is a way to set no-caching for the fetch request. We might need to explore.

As the last option, what we could do is to use an external library like React Query which I think has the option for no-cache. I have used React Query before. But I would first explore the possibility of no-caching with the default fetch request and only if we find that it does not allow/support no-caching, then look into React Query. I think most likely fetch should have some mechanism to set no-caching of the request/response.

@certik
Copy link
Contributor

certik commented Sep 3, 2023

It'd be cool to fix it. It's already very usable, I think this opens up many possibilities. Right now we can compile any single file projects. In Fortran one can concatenate any multi-file project into single-file, so I would stay with this for a while and improve the user experience. If we hook it up with interactive graphics later on, this can be very powerful. People could use this to create demos just by creating gists and posting links. No login required, works everywhere.

@Shaikh-Ubaid
Copy link
Member

Sounds Great, Ondrej Sir!

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.

Give an error message for incorrect url
3 participants