My .js.orig files are being included since I upgraded from .8.5 or so to .9.5 #279

Closed
jkodroff opened this Issue Feb 26, 2014 · 16 comments

Comments

Projects
None yet
3 participants
@jkodroff
Contributor

jkodroff commented Feb 26, 2014

I recently upgraded to SquishIt.latest and now my git merge leftovers (.js.orig) are being included. Here's the line whose behavior has changed:

Bundle.JavaScript().Add("~/path/to/somefiles")

Not sure whether this is a bug or I'm doing it wrong, but figured it would be helpful to have it documented either way.

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Feb 26, 2014

Collaborator

I guess you're kind of doing it wrong, but the wrongest thing you're doing is keeping the .orig files around.

Basically, starting with v0.9 we ripped preprocessing out of the core logic and moved to a system that allows preprocessor chaining (see here). Because the chaining is controlled by file extension, folder resolution was changed to include any files in the folder that have any of the allowed extensions (just .js if you haven't registered any preprocessors) anywhere in the path after the first period (this portion is split on '.' so .jsx for example would not be included). I am not positive this is the correct behavior but it is the behavior for now.

Collaborator

AlexCuse commented Feb 26, 2014

I guess you're kind of doing it wrong, but the wrongest thing you're doing is keeping the .orig files around.

Basically, starting with v0.9 we ripped preprocessing out of the core logic and moved to a system that allows preprocessor chaining (see here). Because the chaining is controlled by file extension, folder resolution was changed to include any files in the folder that have any of the allowed extensions (just .js if you haven't registered any preprocessors) anywhere in the path after the first period (this portion is split on '.' so .jsx for example would not be included). I am not positive this is the correct behavior but it is the behavior for now.

@jkodroff

This comment has been minimized.

Show comment Hide comment
@jkodroff

jkodroff Feb 27, 2014

Contributor

I agree with you that leaving the .orig files is wrong. I deleted them and I'm fine with the current behavior for now, but what do you think of allowing file masks (possibly via a method with a different name) instead? Something like this:

Bundle.JavaScript().AddWildcard("~/path/to/somefiles/*.js")

Contributor

jkodroff commented Feb 27, 2014

I agree with you that leaving the .orig files is wrong. I deleted them and I'm fine with the current behavior for now, but what do you think of allowing file masks (possibly via a method with a different name) instead? Something like this:

Bundle.JavaScript().AddWildcard("~/path/to/somefiles/*.js")

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Feb 27, 2014

Collaborator

I kind of think what it does right now is pretty smart (looks for any of the preprocessors indicated by the file path and registered preprocessor types). I would prefer not to do anything that bypasses that. This is intended to work a lot like rails' asset pipeline, and the issue you reported exposed a flaw in the implementation. A friend told me today that a file with a name like:

alex.coffee.badextension.erb

Would only be preprocessed by erb. So when we calculate the valid preprocessors we should start at the end (in your case "orig"), see that there is no preprocessor registered for that extension and return immediately. It would be my strong preference to match what rails does, and it seems ridiculous that someone could name their files some nonsense like the stuff we're talking about without the express intent of stopping preprocessing (eg alex.test.coffee.erb.inactive).

I think I'm willing to risk causing people a little pain to get this right. I'll try to at least get a prerelease package out soon.

Collaborator

AlexCuse commented Feb 27, 2014

I kind of think what it does right now is pretty smart (looks for any of the preprocessors indicated by the file path and registered preprocessor types). I would prefer not to do anything that bypasses that. This is intended to work a lot like rails' asset pipeline, and the issue you reported exposed a flaw in the implementation. A friend told me today that a file with a name like:

alex.coffee.badextension.erb

Would only be preprocessed by erb. So when we calculate the valid preprocessors we should start at the end (in your case "orig"), see that there is no preprocessor registered for that extension and return immediately. It would be my strong preference to match what rails does, and it seems ridiculous that someone could name their files some nonsense like the stuff we're talking about without the express intent of stopping preprocessing (eg alex.test.coffee.erb.inactive).

I think I'm willing to risk causing people a little pain to get this right. I'll try to at least get a prerelease package out soon.

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Feb 27, 2014

Collaborator

@jincod behavior for your *hogan.html templates would be affected by this. Would it be a problem for you to use html.hogan as a naming convention instead?

Collaborator

AlexCuse commented Feb 27, 2014

@jincod behavior for your *hogan.html templates would be affected by this. Would it be a problem for you to use html.hogan as a naming convention instead?

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Feb 27, 2014

Collaborator

Right now the way I'm approaching this is to add a null preprocessor that can be registered to prevent specified extensions from blocking the preprocessing pipeline (another example is the .less.css convention). I can see the .hogan.html being another good one to use (so you get syntax highlighting in visual studio) so I'm including that as well.

We can register the default extensions (css, js, html) like so:

internal static readonly List<IPreprocessor> Preprocessors = new List<IPreprocessor> { new NullPreprocessor(".JS", ".CSS", ".HTML") };

You could then call Bundle.RegisterGlobalPreprocessor with a NullPreprocessor instance in app_start to specify an extension to "skip" when evaluating preprocessors. So if you wanted test.coffee.fake.erb to be processed by erb then coffeescript you can call

Bundle.RegisterGlobalPreprocessor(new NullPreprocessor(".FAKE"))

This seemed like the best way to allow extension without actually introducing any new weird lists that need to be accounted for in code.

Collaborator

AlexCuse commented Feb 27, 2014

Right now the way I'm approaching this is to add a null preprocessor that can be registered to prevent specified extensions from blocking the preprocessing pipeline (another example is the .less.css convention). I can see the .hogan.html being another good one to use (so you get syntax highlighting in visual studio) so I'm including that as well.

We can register the default extensions (css, js, html) like so:

internal static readonly List<IPreprocessor> Preprocessors = new List<IPreprocessor> { new NullPreprocessor(".JS", ".CSS", ".HTML") };

You could then call Bundle.RegisterGlobalPreprocessor with a NullPreprocessor instance in app_start to specify an extension to "skip" when evaluating preprocessors. So if you wanted test.coffee.fake.erb to be processed by erb then coffeescript you can call

Bundle.RegisterGlobalPreprocessor(new NullPreprocessor(".FAKE"))

This seemed like the best way to allow extension without actually introducing any new weird lists that need to be accounted for in code.

AlexCuse added a commit to AlexCuse/SquishIt that referenced this issue Feb 27, 2014

stop looking for preprocessors after first unmatched extension
- was informed this is rails' behavior
- provide NullPreprocessor mechanism to register extensions that don't
  require preprocessing but should allow preprocessor evaluation to
  continue (ala *.less.css, *.coffee.js, *.hogan.html)
- #279
@AlexCuse

This comment has been minimized.

Show comment Hide comment

AlexCuse referenced this issue Feb 27, 2014

change directory resolution
- require filenames to END WITH an allowed extension for bundle inclusion
- similarly, require filenames to end with a disallowed extension for
  exclusion
@jincod

This comment has been minimized.

Show comment Hide comment
@jincod

jincod Feb 28, 2014

Contributor

@AlexCuse that's not suitable for me, because I already have lots of projects, but I like the option with NullPreprocessor

Contributor

jincod commented Feb 28, 2014

@AlexCuse that's not suitable for me, because I already have lots of projects, but I like the option with NullPreprocessor

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Feb 28, 2014

Collaborator

Cool. I think I am going to try to make this a little smarter before releasing it (so that .css, .js, .html aren't skipped by default).

Would an optional parameter for extensions to ignore on the preprocessor registration methods work? Like

Bundle.RegisterStylePreprocessor(new HoganPreprocessor(), new[] { ".HTML" }) work for you?

I thought about driving this off the preprocessor itself, but that feels like it is assuming a bit too much about how people are using it.

Collaborator

AlexCuse commented Feb 28, 2014

Cool. I think I am going to try to make this a little smarter before releasing it (so that .css, .js, .html aren't skipped by default).

Would an optional parameter for extensions to ignore on the preprocessor registration methods work? Like

Bundle.RegisterStylePreprocessor(new HoganPreprocessor(), new[] { ".HTML" }) work for you?

I thought about driving this off the preprocessor itself, but that feels like it is assuming a bit too much about how people are using it.

@jincod

This comment has been minimized.

Show comment Hide comment
@jincod

jincod Feb 28, 2014

Contributor

Yes. Great idea.
And NullPreprocessor would not be necessary.

Contributor

jincod commented Feb 28, 2014

Yes. Great idea.
And NullPreprocessor would not be necessary.

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Mar 10, 2014

Collaborator

I'm working on adding an "IgnoreExtensions" property to IPreprocessor - this should allow everything to keep working without any change. Its kind of a pain but I think it should work.

Collaborator

AlexCuse commented Mar 10, 2014

I'm working on adding an "IgnoreExtensions" property to IPreprocessor - this should allow everything to keep working without any change. Its kind of a pain but I think it should work.

AlexCuse added a commit to AlexCuse/SquishIt that referenced this issue Mar 10, 2014

AlexCuse added a commit to AlexCuse/SquishIt that referenced this issue Mar 10, 2014

@jkodroff

This comment has been minimized.

Show comment Hide comment
@jkodroff

jkodroff Mar 13, 2014

Contributor

It's not bothering me, and I'm think you had it right originally - that I
was doing it wrong by not deleting those files.

On Sun, Mar 9, 2014 at 9:06 PM, Alex Ullrich notifications@github.comwrote:

I'm working on adding an "IgnoreExtensions" property to IPreprocessor -
this should allow everything to keep working without any change. Its kind
of a pain but I think it should work.

Reply to this email directly or view it on GitHubhttps://github.com/jetheredge/SquishIt/issues/279#issuecomment-37146464
.

Contributor

jkodroff commented Mar 13, 2014

It's not bothering me, and I'm think you had it right originally - that I
was doing it wrong by not deleting those files.

On Sun, Mar 9, 2014 at 9:06 PM, Alex Ullrich notifications@github.comwrote:

I'm working on adding an "IgnoreExtensions" property to IPreprocessor -
this should allow everything to keep working without any change. Its kind
of a pain but I think it should work.

Reply to this email directly or view it on GitHubhttps://github.com/jetheredge/SquishIt/issues/279#issuecomment-37146464
.

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Mar 13, 2014

Collaborator

It did expose a deeper issue with the way we apply preprocessors not matching rails behavior (which is what we were shooting for). There aren't any preprocessors that are currently very useful for chaining but I can see an ERB-esque template processor being useful in the future for easy theming - when I get around to that it will be nice to have this fixed.

Sometimes improvement comes from the silliest things :)

Collaborator

AlexCuse commented Mar 13, 2014

It did expose a deeper issue with the way we apply preprocessors not matching rails behavior (which is what we were shooting for). There aren't any preprocessors that are currently very useful for chaining but I can see an ERB-esque template processor being useful in the future for easy theming - when I get around to that it will be nice to have this fixed.

Sometimes improvement comes from the silliest things :)

@jkodroff

This comment has been minimized.

Show comment Hide comment
@jkodroff

jkodroff Mar 13, 2014

Contributor

Well in that case... can we make it the default behavior... CAN HAZ?
Pretty pleez?

On Thu, Mar 13, 2014 at 7:10 PM, Alex Ullrich notifications@github.comwrote:

It did expose a deeper issue with the way we apply preprocessors not
matching rails behavior (which is what we were shooting for). There aren't
any preprocessors that are currently very useful for chaining but I can see
an ERB-esque template processor being useful in the future for easy theming

  • when I get around to that it will be nice to have this fixed.

Sometimes improvement comes from the silliest things :)

Reply to this email directly or view it on GitHubhttps://github.com/jetheredge/SquishIt/issues/279#issuecomment-37598377
.

Contributor

jkodroff commented Mar 13, 2014

Well in that case... can we make it the default behavior... CAN HAZ?
Pretty pleez?

On Thu, Mar 13, 2014 at 7:10 PM, Alex Ullrich notifications@github.comwrote:

It did expose a deeper issue with the way we apply preprocessors not
matching rails behavior (which is what we were shooting for). There aren't
any preprocessors that are currently very useful for chaining but I can see
an ERB-esque template processor being useful in the future for easy theming

  • when I get around to that it will be nice to have this fixed.

Sometimes improvement comes from the silliest things :)

Reply to this email directly or view it on GitHubhttps://github.com/jetheredge/SquishIt/issues/279#issuecomment-37598377
.

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Mar 13, 2014

Collaborator

It should be the default behavior with these pre-release versions - are you seeing otherwise?

Collaborator

AlexCuse commented Mar 13, 2014

It should be the default behavior with these pre-release versions - are you seeing otherwise?

@jkodroff

This comment has been minimized.

Show comment Hide comment
@jkodroff

jkodroff Mar 13, 2014

Contributor

Haven't tested yet. I need some dumbass on my team to leave the .orig
files around. ;)

On Thu, Mar 13, 2014 at 7:32 PM, Alex Ullrich notifications@github.comwrote:

It should be the default behavior with these pre-release versions - are
you seeing otherwise?

Reply to this email directly or view it on GitHubhttps://github.com/jetheredge/SquishIt/issues/279#issuecomment-37600075
.

Contributor

jkodroff commented Mar 13, 2014

Haven't tested yet. I need some dumbass on my team to leave the .orig
files around. ;)

On Thu, Mar 13, 2014 at 7:32 PM, Alex Ullrich notifications@github.comwrote:

It should be the default behavior with these pre-release versions - are
you seeing otherwise?

Reply to this email directly or view it on GitHubhttps://github.com/jetheredge/SquishIt/issues/279#issuecomment-37600075
.

@AlexCuse

This comment has been minimized.

Show comment Hide comment
@AlexCuse

AlexCuse Mar 29, 2014

Collaborator

I'm going to go ahead and close this - fairly sure it will work now :)

Collaborator

AlexCuse commented Mar 29, 2014

I'm going to go ahead and close this - fairly sure it will work now :)

@AlexCuse AlexCuse closed this Mar 29, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment