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

Restore broken *express-minify* ability to minify everything #434

Merged
merged 1 commit into from Nov 21, 2014

Conversation

Martii
Copy link
Member

@Martii Martii commented Nov 21, 2014

  • Apparently order is important again here and Make express-minify optional for dev #431 didn't take into account this by skipping regression testing
  • Alter the routine to restore the order and the exception of not using require at the top is rescinded for this instance.
  • try..catch is still used for those devs with misconfigured environments

Related:

* Apparently order is important again here and OpenUserJS#431 didn't take into account this by skipping regression testing
* Alter the routine to restore the order and the exception of not using `require` at the top is rescinded
* `try..catch` is still used for those devs with misconfigured environments
Martii added a commit that referenced this pull request Nov 21, 2014
Restore broken *express-minify* ability to minify everything

Auto-merge
@Martii Martii merged commit baa8cd5 into OpenUserJS:master Nov 21, 2014
@Martii Martii deleted the restoreExpressMinify branch November 21, 2014 03:02
@Zren
Copy link
Contributor

Zren commented Nov 21, 2014

Apparently order is important again here

The order changed in that commit?!? You're not even moving the middleware. Unless you need to require() the express-minify module before everything else? If that is the case, where's the comment specifying that? What specifically broke? Why is that happening?

skipping regression testing

And like Jerone said, you broke the logic. It will now break on isDev without express-minify installed.

@Martii
Copy link
Member Author

Martii commented Nov 21, 2014

It's already been confirmed that we both foo'd... mine was a late night whoops (which was briefly thought of as a spark in my synapses to actually put parens around it)... yours is well a personal reason as I've stated before in #431. @jerone's approach is a lot better than yours most of the time and he's right not you. :) I'm still also recovering from your violation of the ToS in which I almost booted you from the project (and yes @sizzlemctwizzle knows this) but now that we're even for my mistake a few months ago try to be a little more professional and lot less vindictive/sarcastic/etc. please... otherwise he'll make a judgement call that I can't.

@Zren
Copy link
Contributor

Zren commented Nov 21, 2014

What specifically broke?

Answer the question please.

recovering from your violation of the ToS

Eh? What violation?

Martii pushed a commit to Martii/OpenUserJS.org that referenced this pull request Nov 21, 2014
Reapplies to:
* OpenUserJS#434
* OpenUserJS#431

Eventually this test should go away when @Zren takes the necessary time to reinstall his Windows machine instead of making a personal exception since his machine is misconfigured.
@Martii Martii mentioned this pull request Nov 21, 2014
@Martii
Copy link
Member Author

Martii commented Nov 21, 2014

You are being snarky as hell and he knows and I know it... but I asked you nicely to keep your bad attitude out of the project instead of booting you... e.g. that's a warning.

@Martii
Copy link
Member Author

Martii commented Nov 21, 2014

Okay spell out time with a reverse question (which is part of regression testing)....

You asked

What specifically broke?

This subject is

Restore broken express-minify ability to minify everything

What two tests do we currently have that forced minification to NOT occur recently?

@Zren
Copy link
Contributor

Zren commented Nov 21, 2014

You are saying that my commit had a bug in it. It would be extremely helpful if you could point out what that bug was so that I will not make it a second time.

Your comments mention "Apparently order is important again here", does that mean: "need to require() the express-minify module before everything else"?

@Martii
Copy link
Member Author

Martii commented Nov 21, 2014

I didn't specifically diagnose express-minify but my educated guess based off the results of not being able to minify everything is that it needs to hook into the request before one of our other dependencies on "init" and "start" later so Ace doesn't throw the exception.

@Zren
Copy link
Contributor

Zren commented Nov 21, 2014

It should not matter when you require() a middleware module. It doesn't actually get pushed into the array of middlewares until you insert it with app.use(). Did you notice this bug on production? Or in your local env?

@Martii
Copy link
Member Author

Martii commented Nov 21, 2014

It should not matter when you require() a middleware module.

But it does matter apparently (hence my commit message)... I didn't design 100% of node and it's dependencies/projects. Feel free to figure this one out but keep minification in for everything with an opt out in our source now that we have it.

Did you notice this bug on production? Or in your local env?

Multiple local environments of different capacities (CPU/MEM/etc). A few more tests will be calculated for production which is part of #249

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants