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

middleware: don't use middleware when loading images #429

Merged
merged 1 commit into from Jul 15, 2019

Conversation

JoeGruffins
Copy link
Member

check url for gets directed at "/assets" and "/captchas" and ignore them
in the middleware

before this change, open home, fmt.Println("ApplyDbMap") in middleware.go func ApplyDbMap
before

after this change, open home with the same print in the same place
after
I don't believe middleware should be applied to gets to pics for performance reasons, as they aren't using the information, I don't think

@JoeGruffins
Copy link
Member Author

Actually, I think I have a better answer as soon as I figure out the correct negated regex expression.

@JoeGruffins JoeGruffins changed the title middleware: don't use middleware when loading images [WIP] middleware: don't use middleware when loading images Jul 1, 2019
@jholdstock
Copy link
Member

Maintaining a regex here is not appropriate because it will require changes every time the application routes change, which is going to be very error-prone and non-intuitive for new devs.

This can be achieved by simply creating a new sub-router for static files, and don't attach any middleware to that subrouter.

I actually implemented this change some time ago, but it seems like it never made it into master. It must be lost on my local machine somewhere, I will try to find it.

@JoeGruffins
Copy link
Member Author

ok! That's what I'm trying to do, but then the regex wouldn't let me do not, and I'm not sure how to specify the folders easily otherwise, yet.

@jholdstock
Copy link
Member

https://github.com/decred/dcrstakepool/compare/master...jholdstock:routers?expand=1

Found it - see the changes here. I think this achieves the desired result.

@jholdstock
Copy link
Member

Note that /robots.txt and /favicon.ico are not required, they do not actually exist at these locations

@JoeGruffins
Copy link
Member Author

thanks, I just needed to change the order and goji does the rest. Ill remove those paths then?

server.go Outdated Show resolved Hide resolved
-divide html routes into html and static files
-don't apply middleware to static files
@JoeGruffins JoeGruffins changed the title [WIP] middleware: don't use middleware when loading images middleware: don't use middleware when loading images Jul 4, 2019
@dajohi dajohi requested a review from jholdstock July 8, 2019 14:47
Copy link
Member

@jholdstock jholdstock 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. Thanks

@dajohi dajohi merged commit 43d6cbd into decred:master Jul 15, 2019
@isuldor
Copy link
Contributor

isuldor commented Jul 17, 2019

This is a good improvement. I just noticed dcrdata defines an nginx location to serve static resources directly:

# Serve static resources directly.
location ~ ^/(css|js|images|fonts|dist|index.js) {
        gzip_static on; # use .gz files for pre-compressed data
        root /opt/dcrdata/public;
        # Set the Cache-Control and Expires headers for the static assets.
        expires modified 2d;
}

@chappjc
Copy link
Member

chappjc commented Jul 17, 2019

That's a solution I would encourage, especially since nginx is so effective at handling cached static resources.

@JoeGruffins
Copy link
Member Author

like here ? #433

@chappjc
Copy link
Member

chappjc commented Jul 17, 2019

Oh, good stuff. ;)

@JoeGruffins
Copy link
Member Author

I think #433 is missing an opening bracket.

@JoeGruffins
Copy link
Member Author

hey, thanks guyz, quick question. @chappjc @isuldor

So with this in the nginx conf:

    # Serve static resources directly.
    location /assets/*
            gzip_static on; # use .gz files for pre-compressed data
            root /opt/dcrstakepool/public;
            # Set the Cache-Control and Expires headers for the static assets.
            expires modified 2d;
    }

Should this pr we are commenting on now not be necessary? I can't tell that the above makes any difference. Even adding the bracket after location /assets/*

@JoeGruffins
Copy link
Member Author

Actually, I think I get it now. Thank you!

@JoeGruffins JoeGruffins deleted the lessmiddleware branch August 6, 2019 12:03
girino added a commit to girino/dcrstakepool that referenced this pull request Sep 7, 2019
* commit 'bbcce9335b3eafca8469796d48e2f2649d17451c':
  go.mod: update protoc-gen-go to version 3 (decred#436)
  main.go: get for voting page with no multisig redirects to address (decred#438)
  emailupdate: emailupdate to same design as passwordupdate (decred#435)
  server: don't use middleware when loading images (decred#429)
  Remove duplicated js-only code (decred#439)
jyap808 pushed a commit to ubiq/dcrstakepool that referenced this pull request Dec 12, 2019
-divide html routes into html and static files
-don't apply middleware to static files
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

5 participants