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

Add option for outputFilename #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add option for outputFilename #4

wants to merge 4 commits into from

Conversation

karlhorky
Copy link
Contributor

No description provided.

@TiddoLangerak
Copy link
Contributor

I like the idea of this, but this doesn't work well when you have multiple css files. I think it would be a good idea to add support for predefined variables as well, e.g. [name] and [basename], similar as to how the extract-text-webpack-plugin does this.

@karlhorky
Copy link
Contributor Author

Sure, that sounds like a good idea. I'll take a look and see if I can devise something along these lines.

@karlhorky
Copy link
Contributor Author

Okay, I gave it a shot. What do you think? First time writing webpack plugin code, so not sure I'm doing it right.

@karlhorky
Copy link
Contributor Author

Any feedback? I can also do it differently, if this is not what you had in mind.

basename = name.substring(name.lastIndexOf('/') + 1);
if (basename.lastIndexOf('.') !== -1) {
basename = basename.substring(0, basename.lastIndexOf('.'));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use path.parse instead? Not all operating systems (i.e. Windows) use / as a path separator, using the NodeJS provided modules here gives cross platform support for free.

@TiddoLangerak
Copy link
Contributor

Sorry for the late reply, I was a bit pre-occupied. Other than the comment I gave it looks great! If you update it then I'll test and release it tomorrow.

@karlhorky
Copy link
Contributor Author

No problem, I've solved this with path.basename() instead, does that work?

@TiddoLangerak
Copy link
Contributor

Sure, that's even better!

@karlhorky
Copy link
Contributor Author

Great, look forward to the tests / release. I didn't go ahead with writing tests for this because the plugin has been rebuilt in #2 anyway. I've added this as a comment for the other branch too.

@karlhorky
Copy link
Contributor Author

Any luck?

@karlhorky
Copy link
Contributor Author

@TiddoLangerak I guess this is not going to be merged, right?

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.

2 participants