lukeed / taskr

A fast, concurrency-focused task automation tool.
MIT License
2.53k stars 74 forks source link

Files are not written #237

Closed hzlmn closed 7 years ago

hzlmn commented 7 years ago

I have such task, when I want to place files to the same folder, files are not written. If I will change target path to something like ${main}/scss/gl-uikit/test it will work and files will be generated.

const main = 'app/ui-kit'

x.icongen = function * () {
  yield this
    .source(`${main}/scss/gl-uikit/glfont.scss`)
    // Plugin that generates fontvars - 2 files
    .icongen()
    .target(`${main}/scss/gl-uikit/`)
}
lukeed commented 7 years ago

@hzlmn What does the plugin look like?

hzlmn commented 7 years ago

Hi, sorry for delay. Plugin reads custom icon file css file and generate 2 new files with variables.

module.exports = function () {
  this.plugin('icongen', {}, function * (file, opts) {
    var data = file.data.toString()

    if (!data.length) {
      this.emit('plugin_error', {
        plugin: 'fly-icon',
        error: 'File could not be empty'
      })
    }

    // params
    var fontVarsName = opts.fontVarsName || 'glfontvars'
    var fontListName = opts.fontListName || 'glfontvars-list'
    var ext = opts.ext || '.scss'

    const model = getModel(data)

    const files = [
      {name: fontVarsName, data: createFontVarsFile(model)},
      {name: fontListName, data: createFontListFile(model)}
    ]

    // Pop glfont item
    this._.files.pop()

    files.map(f => this._.files.push({
      base: f.name.concat(ext),
      dir: file.dir,
      data: new Buffer(f.data)
    }))
  })
}
lukeed commented 7 years ago

@hzlmn No problem. Here's your problem:

  1. You are using pop() which is removing each file ({every: 1} is the default) from the global fly._.files array. So by the time this plugin is done, you have 0 files left in your array.

    const arr = [1, 2, 3, 4]
    
    const p1 = arr.pop(); 
    //=> 4
    
    console.log(arr);
    //=> [1,2,3]

    Also, it does not make sense to use this method. Fly traverses from 0 -> 4 while pop() will simultaneously be removing 4 -> 0. You'll only process half your files this way.

  2. Secondly, you never write the new data back into your file or files array. At the very end, you should have been doing this:

    this._.files = files.map(f => this._.files.push({
       base: f.name.concat(ext),
       dir: file.dir,
       data: new Buffer(f.data)
    }))

    However, that alone won't fix your plugin, because of pop().

Possible Replacement

Since it seems like you want to just read a bunch of files & generate entirely different output (without affecting the original files), you should be using {every: 0}. This way you can prepare and finalize the files array, as well as control when the files-loop happens.

Try this:

this.plugin('icongen', {every: 0}, function * (files, opts) {
  // params, does not change
  const vars = opts.fontVarsName || 'glfontvars'
  const list = opts.fontListName || 'glfontvars-list'
  const ext = opts.ext || '.scss'

  // prepare output array
  const out = []

  for (const f of files) {
    const data = f.data.toString()
    if (!data) continue;
    const model  = getModel(data)
    // add to our output array
    out = out.concat([{
      base: vars.concat(ext), 
      dir: f.dir,
      data: new Buffer(createFontVarsFile(model))
    }, {
      base: list.concat(ext), 
      dir: f.dir,
      data: new Buffer(createFontListFile(model))
    }])
  }

  // now replace files array
  this._.files = out
})
hzlmn commented 7 years ago

Hi, sorry maybe I missunderstood you, but

You are using pop() which is removing each file ({every: 1} is the default) from the global fly._.files array. So by the time this plugin is done, you have 0 files left in your array.

Yes I know, but in my case it will be always only one file glfont.css, then i pop this file, because plugin will work only once since we have single file, and then push new items back to this._.files array.

 files.map(f => this._.files.push({
    base: f.name.concat(ext),
    dir: file.dir,
    data: new Buffer(f.data)
 }))

So when target function is called, I got correct output here

files [ { base: 'glfontvars.scss',
    dir: '/home/olehkuchuk/Projects/gl-uikit/app/ui-kit/scss/gl-uikit',
    data: <Buffer 24 67 6c 69 63 6f 6e 2d 70 6c 61 79 2d 76 69 64 65 6f 3a 20 22 65 39 34 38 22 3b 0a 24 67 6c 69 63 6f 6e 2d 6e 75 6d 62 65 72 65 64 2d 6c 69 73 74 3a ... > },
  { base: 'glfontvars-list.scss',
    dir: '/home/olehkuchuk/Projects/gl-uikit/app/ui-kit/scss/gl-uikit',
    data: <Buffer 0a 20 20 20 20 24 66 6f 6e 74 2d 69 63 6f 6e 2d 76 61 72 69 61 62 6c 65 73 3a 20 28 0a 20 20 20 20 20 20 70 6c 61 79 2d 76 69 64 65 6f 3a 20 24 67 6c ... > } ]

Am I missing something?

lukeed commented 7 years ago

and then push new items back to this._.files array.

It won't push them back anywhere. You created your own files variable here:

const files = [
    {name: fontVarsName, data: createFontVarsFile(model)},
    {name: fontListName, data: createFontListFile(model)}
]

When you map files, it won't affect anything. You don't even have it assigned to a new variable~

// what you're doing: 
let files = [1, 2, 3, 4];

// you map the files
files.map(n => n * 2);
//=> [2, 4, 6, 8]

// but 'files' is unaffected
console.log(files);
//=> [1, 2, 3, 4]

// TRY AGAIN, but assign to variable
files = files.map(n => n * 2);

console.log(files);
//=> [2, 4, 6, 8]

Does that make sense?

So when target function is called, I got correct output here

Nothing in your plugin is creating or attaching files to Fly. You are only removing files via pop(). I believe another task or another plugin is sourcing or creating those two files, and that's why you think the target() array is correct.

Did you try my plugin? It works


Also, depending on the time of day, I'd be able to help you debug further via screenshare on Skype or something.

hzlmn commented 7 years ago

@lukeed

When you map files, it won't affect anything. You don't even have it assigned to a new variable

Yes, I create new files array, then map on it but inside map callback I push each file back to this._.files. Look here

files.map(f => this._.files.push({ // << here I push new file item
    base: f.name.concat(ext),
    dir: file.dir,
    data: new Buffer(f.data)
 }))
lukeed commented 7 years ago

Oh, I overlooked that part. What is your task's source?

hzlmn commented 7 years ago

Not sure what you mean

x.icongen = function * () {
  yield this
    .source(`${main}/scss/gl-uikit/glfont.scss`)
    .icongen()
    .target(`${main}/scss/gl-uikit/`)
}

Here is task, I only sourcing one file glfont.scss

lukeed commented 7 years ago

Sorry @hzlmn, I can't reproduce your issue 😞 Your original plugin is working for me.

Here's what I have:

const getModel = d => d
const createFontVarsFile = d => d
const createFontListFile = d => d

module.exports = function () {
  this.plugin('icongen', {}, function * (file, opts) {
    var data = file.data.toString()

    if (!data.length) {
      this.emit('plugin_error', {
        plugin: 'fly-icon',
        error: 'File could not be empty'
      })
    }

    // params
    var fontVarsName = opts.fontVarsName || 'glfontvars'
    var fontListName = opts.fontListName || 'glfontvars-list'
    var ext = opts.ext || '.scss'

    const model = getModel(data)

    const files = [{
      name: fontVarsName,
      data: createFontVarsFile(model)
    }, {
      name: fontListName,
      data: createFontListFile(model)
    }]

    // Pop glfont item
    this._.files.pop()

    files.map(f => this._.files.push({
      base: f.name.concat(ext),
      dir: file.dir,
      data: new Buffer(f.data)
    }))
  })
}
const main = 'app/ui-kit';

exports.icongen = function * () {
  yield this.source(`${main}/scss/gl-uikit/glfont.scss`)
    .icongen().target(`${main}/scss/gl-uikit/`);
}

What Node version are you on this time? And what OS?

hzlmn commented 7 years ago

@lukeed Hm, interesting. I use Node 6.2.0 and Ubuntu 14 LTS. I will look on it under MacOS, maybe It's some kind of problem with writing permissions on Linux, but I hardly think, that such error can be silent.

hzlmn commented 7 years ago

Btw, thanks for your time and quick responses.

lukeed commented 7 years ago

No problem. I've been thinking about it too. I'll try on node 6.2

lukeed commented 7 years ago

@hzlmn Works on 6.2.0

On your Ubuntu machine, try touch file.js anywhere and see if it lets you. My VM requires sudo for those actions, so you may need to do the same.

hzlmn commented 7 years ago

@lukeed Seems stupid, but it works after reboot :smile: Closing it.

lukeed commented 7 years ago

@hzlmn Haha, odd 😜