lukeed / taskr

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

target structure is not correct when passing `file` as param to task #216

Closed devmondo closed 7 years ago

devmondo commented 7 years ago

i hope i am not doing something wrong,

this issue #200 was fixed and works perfectly now, but when using fly-babel if you dont pass file as param to the task function babel will transpile all files in path.

As i understood from the doc watch file should be passed, and it works great now babel only transpile the passed file, but the same problem in the referenced issue above occurs again.

here is my code

const paths = {
    scripts: ['./src/**/*.js']
}

export default async function () {
    await this.watch(paths.scripts, `babel`);
}

export async function babel(file) {
    await  this.source(file || paths.scripts)
        .babel({
            presets: [["es2015", {"modules": false}], "stage-1", "stage-3"],
            plugins: ["transform-es2015-modules-commonjs",
                ['transform-runtime', {
                    "polyfill": false,
                    "regenerator": true
                }]
            ],
            sourceMaps: true
        })
        .target(`./dist`);
 }
lukeed commented 7 years ago

Windows, right?

devmondo commented 7 years ago

yep

lukeed commented 7 years ago

@devmondo Sorry bud, busy day today. I'll get to it soon!

I know the culprit is in #189, just gotta comb thru & find it.

devmondo commented 7 years ago

@lukeed no issues mate, i really appreciate the care :)

lukeed commented 7 years ago

I have this solved in my local development of fly@2.0, but it's not a direct fit to the current 1.4.

So... useless update for you 😆

devmondo commented 7 years ago

hahahha i have to wait then :)

hzlmn commented 7 years ago

@lukeed can you push your branch, so we can start involving into it?

devmondo commented 7 years ago

the big problem for me now is that because i cant compile one file only every other task is slowed down like browser-sync reload, because it has to wait for babel to be done.

lukeed commented 7 years ago

@hzlmn Yes of course, but not quite yet. There's no functional base yet, but I plan to resolve it all today or tomorrow.

@devmondo I'll take a look today!

devmondo commented 7 years ago

@lukeed thaks man did not mean to rush, sorry if it sounded like that.

lukeed commented 7 years ago

@devmondo all is well. something like that would annoy the heck outta me

lukeed commented 7 years ago

@devmondo So I rebuilt your environment on OSX and this is what's happening to me.

\src
  |-- app.js
  |-- \a
    |-- a-1.js
    |-- a-2.js
  |-- \b
    |-- b-1.js
    |-- b-2.js

Everything builds correctly on the first cycle.

Then, while the fly.watch is running, I save src/b/b-2.js which creates a new file at dist/b-2.js instead of dist/b/b-2.js.

Although this is still an issue, is this the same issue you're encountering? I got the impression that yours was creating a new file at dist/src/b/b-2.js, but perhaps I misunderstood.

devmondo commented 7 years ago

well basically it is exactly the same problem as in #200

\src
 |--index.js
//should result in 
\dist
 |--index.js
//but this is what happen
\dist
 |--src
    |--index.js

and as you mentioned everything is fine on first cycle. and from what you discribed it seems that your issue is the opposite on OSX!

lukeed commented 7 years ago

Releasing as 1.4.1. Give it a spin && let me know!

I tested on windows with 5-level nesting, worked like it's supposed to. Thanks for reporting :)

devmondo commented 7 years ago

@lukeed it works like charm and now browser-sync and compiling happen in 200 ms instead of 1.2 seconds!

i will continue testing to see if there are other edge cases.

thank you very much.

devmondo commented 7 years ago

sorry to re post again, but it seems that the bug still exist, we did not notice that because we had this structure

|-src
  |-index.js

but now we have this structure

|-src
  |-client  
     |-index.js

first time it runs it is ok as usual, but second time it becomes like this

|-dist
  |-client  
     |-client  
        |-index.js
lukeed commented 7 years ago

I think I know why... your paths all began with ./ which will mess up the index count (not the best way to handle it, but was best I could come up with in current environment).

So change './src/...' and './dist/...' to 'src/...' and 'dist/...'

At the time, I tested with:

\src
  |-- _.js
  |-- \lvl1
  |-- a.js
    |-- \lvl2
    |-- b.js
      |-- \lvl3
      |-- c.js
        |-- \lvl4
        |-- d.js

And the output tree was consistently built.

If that doesn't work for you, I'll have to hijack a windows computer again and tinker some more.

devmondo commented 7 years ago

still not working?

const paths = {
    clientScripts: [`src/client/**/*.js`]
}

export default async function () {
   await this.watch(paths.clientScripts, `clientJs`);
}

export async function clientJs(file) {
    await  this.source(file || paths.clientScripts)
        .babel(babelOptions)
        .target(`dist/client`);
}

hope i am not doing anything wrong!

lukeed commented 7 years ago

Nope, looks fine. I'll hijack a machine.

lukeed commented 7 years ago

@devmondo Update:

It's not working because when watch event triggers, all of source's known globs are reset as the full file path is sent. So, target has no context to compare the full file path against.

On first builds, it has the globs & thus looks for the segment in which the first wildcard (*) appears, if any, otherwise it assumes that the first segments, usually 'src/' is supposed to be dropped.

Since the globs are lost during watch events, target is assuming that just the first segment is to be dropped. That is src in your case, but that means that the rest of the path (client/sub/index.js) is kept & forwarded to the destination.

That's also why broader patterns (eg, src/**/*.js) work... dropping src is all that needs to be done.

Long story short: It works according to design, but the design is lacking. Need to retain the _.globs value during watch updates.

devmondo commented 7 years ago

@lukeed thanks for the update 👍

shameful hack for now that worked!

export async function clientJs(file) {
    let source = file ? file : paths.clientScripts;
    let target = file ? `dist/` : `dist/client/`;
    await  this.source(source)
        .babel(babelOptions)
        .target(target);
}

hope this will be fixed soon :)

lukeed commented 7 years ago

Yeah, that's what I was going to suggest for the time being. Can't get my hands on the Windows machine again for a little bit.