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.

Sign up for GitHub

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

Jump to bottom

Replace url-parse with URL #3671

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Replace url-parse with URL #3671

merged 2 commits into from
Sep 21, 2022

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Sep 17, 2022 •
edited by mcbouslog
Loading

Replace the url-parse package with the native URL interface, which is supported in Node and in browsers. Using the native interface should make both NextJS apps more secure.

From url-parse's Readme:

url-parse was created in 2014 when the WHATWG URL API was not available in Node.js and the URL interface was supported only in some browsers. Today this is no longer true. The URL interface is available in all supported Node.js release lines and basically all browsers. Consider using it for better security and accuracy.

Package

app-content-pages
app-project
lib-panoptes-js

How to Review

Sign in and register should work in both NextJS apps. The project app should still load projects based on the slug in the page URL.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

Refactoring

  • The PR creator has described the reason for refactoring
  • The refactored component(s) continue to work as expected

@eatyourgreens eatyourgreens added the refactor Refactoring existing code label Sep 17, 2022
@eatyourgreens eatyourgreens requested a review from a team September 17, 2022 16:33
@eatyourgreens eatyourgreens force-pushed the replace-url-parse branch 2 times, most recently from 31ec985 to 180ec50 Compare September 17, 2022 20:02
@coveralls
Copy link

coveralls commented Sep 17, 2022 •
edited
Loading

Coverage Status

Coverage decreased (-0.07%) to 82.669% when pulling a440357 on replace-url-parse into 6d9fde2 on master.

@eatyourgreens eatyourgreens force-pushed the replace-url-parse branch 2 times, most recently from 269ca13 to 25603d9 Compare September 17, 2022 21:34
@mcbouslog mcbouslog self-assigned this Sep 20, 2022
mcbouslog
mcbouslog reviewed Sep 20, 2022
View reviewed changes
Copy link
Contributor

@mcbouslog mcbouslog left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code changes look good, but when I run https://local.zooniverse.org:3000/projects/brooke/i-fancy-cats locally (on Chrome) it renders "window is not defined" and there are a few Uncaught Errors, the first of which is Uncaught Error: Target container is not a DOM element..

@mcbouslog
Copy link
Contributor

Code changes look good, but when I run https://local.zooniverse.org:3000/projects/brooke/i-fancy-cats locally (on Chrome) it renders "window is not defined" and there are a few Uncaught Errors, the first of which is Uncaught Error: Target container is not a DOM element..

This Sentry issue is me, related to error noted.

@eatyourgreens
Copy link
Contributor Author

I’ll stage this to kubernetes so that we can take a look. I assume the page just crashes in Node?

@eatyourgreens
Copy link
Contributor Author

Oh, I see the error in the diff. AuthModalContainer is missing a couple of lines in the project app.

eatyourgreens
eatyourgreens commented Sep 20, 2022
View reviewed changes
packages/app-content-pages/src/shared/components/AuthModal/AuthModalContainer.js
return new Url(asPath, true)
getUrlObject() {
const isBrowser = typeof window !== 'undefined'
if (isBrowser) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes also need to be added to the project app.

@eatyourgreens
Copy link
Contributor Author

Ok, this branch is deployed here (with the error):
https://fe-project-branch.preview.zooniverse.org/projects/brooke/i-fancy-cats?env=staging

I'm going to push a change to the AuthModalContainer code and deploy again.

@eatyourgreens
Copy link
Contributor Author

Re-deployed to fe-project-branch but it's still erroring because url.searchParams is undefined on the server.
https://fe-project-branch.preview.zooniverse.org/projects/marshexplorer/marsh-explorer

@eatyourgreens
Copy link
Contributor Author

@mcbouslog I'd made changes to the auth modal in the content pages app but not copied those same changes to the project app. At the moment, the Zooniverse page header has to be edited in both places 😒 . Everything should be working now eg. https://fe-project-branch.preview.zooniverse.org/projects/marshexplorer/marsh-explorer

@eatyourgreens
Copy link
Contributor Author

I'm also not sure why AuthModalContainer is rendered server-side, since you can only log in or register in the browser.

@eatyourgreens
Replace url-parse with URL
a3c95ce 
Replace the `url-parse` package with the native URL interface, which is supported in Node and in browsers.
@eatyourgreens
Use URL objects for redirects
a440357
mcbouslog
mcbouslog approved these changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

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

Looks good! Tested locally, tested on https://fe-project-branch.preview.zooniverse.org/projects/marshexplorer/marsh-explorer, ran app-content locally and confirmed expected sign in and register modals appear 👍 .

@github-actions github-actions bot added the approved This PR is approved for merging label Sep 21, 2022
@eatyourgreens eatyourgreens merged commit 169fed2 into master Sep 21, 2022
@eatyourgreens eatyourgreens deleted the replace-url-parse branch September 21, 2022 06:11
@eatyourgreens eatyourgreens mentioned this pull request Oct 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcbouslog mcbouslog mcbouslog approved these changes

Assignees

@mcbouslog mcbouslog

Labels
approved This PR is approved for merging refactor Refactoring existing code
Projects
Maintenance
Status: Done
fe-project-branch deploy
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eatyourgreens @coveralls @mcbouslog