andywer / postcss-theme

PostCSS plugin to enable versatile theming.
MIT License
87 stars 2 forks source link

themePath - conversion of forward slash to back slash causes incompatibility on Windows OS #1

Closed lordy81 closed 8 years ago

lordy81 commented 8 years ago

In a windows environment, using webpack.

For the given configuration:

postcss: function () {
    return [
      theme({ themePath: 'c:/project/ui/src/styles/themes/default'}),
      values,
      autoprefixer
    ]
  }

When used with the following in line in css:

@value tooltip-text from theme(themecolors);

The module is resolved to the following directory:

c:\project\ui\src\styles\themes\default\themecolors.css

Which can't be resolved by windows during compilation.

Module build failed: ModuleNotFoundError: Module not found: Error: Cannot resolve module '"c:\project\ui\src\styles\themes\default\themecolors.css"'

As a test - Hard-coding the path in the css file and using forward slashes works

@value tooltip-text from 'c:/project/ui/src/styles/themes/default';
andywer commented 8 years ago

Hi @lordy81. What path do you expect instead?

Looks like everything is working fine. The import from the hard-coded path c:/project/ui/src/styles/themes/default seems odd. Shouldn't it be c:/project/ui/src/styles/themes/default/some-file.css?

lordy81 commented 8 years ago

Thanks for the quick reply.

Instead of:

c:\project\ui\src\styles\themes\default\themecolors.css

The resolved path needs to be:

c:/project/ui/src/styles/themes/default/themecolors.css

Sorry, you're right I got the hard-coded example wrong - but that was just for illustration.

lordy81 commented 8 years ago

I've narrowed down the transition to where it occurs in themeFileResolver.

It seems path.join is the cause - it is switching the slash direction

path.sep is set to \.

I'll take a look at the path doc to see if this can be changed.

Added logs as follows (Line 22 of index.js in the version I have)

/**
 * @param {object} options { themePath: String }
 */
module.exports = postcss.plugin('postcss-theme', function (options) {
  var themeFileResolver = function (themeFilePath) {
    if (!themeFilePath.match(/\.css$/i)) {
      themeFilePath += '.css'
    }

    console.log("In themeFileResolver");
    console.log("themePath: " + options.themePath);
    console.log("themeFilePath: " + themeFilePath);
    console.log("joined: " + path.join(options.themePath, themeFilePath));

    return path.join(options.themePath, themeFilePath);
  }

Gives this output:

In themeFileResolver themePath: c:/project/ui/src/styles/themes/default themeFilePath: themecolors.css joined: c:\project\ui\src\styles\themes\default\themecolors.css

lordy81 commented 8 years ago

Was looking for uses of path.sep in the other node modules in the project - Have noticed a few which track path.sep and alter it for windows usage as follows:

    // windows support: need to use /, not \
    if (path.sep !== '/') {
      pattern = pattern.split(path.sep).join('/')
    }

Modified themeFileResolver as follows

/**
 * @param {object} options { themePath: String }
 */
module.exports = postcss.plugin('postcss-theme', function (options) {
  var themeFileResolver = function (themeFilePath) {
    if (!themeFilePath.match(/\.css$/i)) {
      themeFilePath += '.css'
    }

    var themeFile = path.join(options.themePath, themeFilePath)
    // windows support: need to use /, not \
    if (path.sep !== '/') {
      themeFile = themeFile.split(path.sep).join('/')
    }
    return themeFile;
  }
andywer commented 8 years ago

Yeah, I've also noticed that it is path.join. Unfortunately I won't have time to fix anything before evening (in a few hours).

But I have one question: Do you really need the slashes? I mean backslashes in paths when using Windows are rather a feature than a bug ;)

lordy81 commented 8 years ago

Save you the time, I've forked the project and added a pull request - let me know what you think.

Its a pain, but we need the forward slash otherwise it won't compile.

fs.stat (used by ModuleAsDirectoryPlugin.js) won't recognise the file with single backslashes.

The comments in other node modules I've looked at say they've made the changes because functionality like 'require' expects a forward slash.

andywer commented 8 years ago

Ahhh. Actually wanted to close this issue, but closed the PR instead... 😅

Have a look at version 1.0.1. Tell me if it works for you!

lordy81 commented 8 years ago

Ah, I can see what you've done, and that's great. :+1:

However - The other issue is that in order to simplify the description of the issue I used a hard-coded string for the themePath. In reality we use path.join as well in the config, which converts to backslashes before it reaches your code.

Personally, I'd prefer the change all instances approach - this would also mean that other developers can use either \ or / in the config.

andywer commented 8 years ago

Hmm... I can totally understand your motivation, but I think this is not the right place to fix this.

But... I am implementing a minor new functionality anyway and maybe you can use that to fix your problem. It will be possible to optionally pass a custom themeFileResolver function as an option. It's basically just a function that replaces the themeFileResolver function in index.js.

Yours could look like that:

function themeFileResolver (themeFilePath, options, originalResolver) {
  return originalResolver(themeFilePath, options).replace(/\\/g, '/')
}

...

postcssTheme({ themePath: './some/path', themeFileResolver })

What do you think?

lordy81 commented 8 years ago

That would work. Brill. I'll watch the project for the update.

Any ETA?

andywer commented 8 years ago

Let's say in about 4h.

andywer commented 8 years ago

Released version 1.1.0