search

marksmccann / node-sass-extra

A drop-in replacement for node-sass' Node API that adds support for globs, promises and more.
MIT License
2 stars 1 forks source link

Harden `isFile` method #46

Open marksmccann opened 5 years ago

marksmccann commented 5 years ago

It's probably a good idea to harden the isFile method in our package. Currently, it is simply checking for a file extension and assuming it is a file, but a directory can have a file extension as well (e.g. 'path/to/dir.css/file.css'). We should probably use stats.isFile() and/or stats.isDirectory() to harden this check, at least for determining the source files.

marksmccann commented 5 years ago

@phillipluther Do you think if we did this, it would solve some of our problems with the CLI? I mean we can more accurately determine if the user is referring to a directory or a file for their source. If it is a directory, we can handle it the same way as the CLI.

phillipluther commented 5 years ago

@marksmccann Without an explicit flag that says, "HEY! I want this file-looking thing to be a directory!" I'm not sure it would help - I mean, if somebody legitimately wants to have a directory called styles.css ... I'm not sure how we'd be able to determine that from a simple string.

If it's in a build and that directory doesn't exist yet, we couldn't check against it. Maybe there's some combination of other settings that would determine if the intent is dir or file?

marksmccann commented 5 years ago

@phillipluther Maybe it is my ignorance, but I was under the impression that stats.isFile() and stats.isDirectory() actually check the file tree to see if the given path is either a file or a directory in actuality, not just looking at the string. If so, wouldn't that work?

marksmccann commented 5 years ago

I haven't tested it yet, but after a quick search, I think that my assumption is correct. If we can accurately determine that the source is a directory, we can then search for sass files inside of it, right?

phillipluther commented 5 years ago

Yes, but when that string comes in as an option it might not exist yet. We’re doing an ‘ensureDir’ to create the path if it doesn’t exist, aren’t we?

I’ll double check that, too. If not, you’re totally right.

From: Mark McCann notifications@github.com Sent: Saturday, September 7, 2019 5:55:43 PM To: marksmccann/node-sass-extra node-sass-extra@noreply.github.com Cc: Phillip Luther dev@phillipluther.com; Mention mention@noreply.github.com Subject: Re: [marksmccann/node-sass-extra] Harden isFile method (#46)

I haven't tested it yet, but after a quick search, I think that my assumption is correct. If we can accurately determine that the source is a directory, we can then search for sass files inside of it, right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmarksmccann%2Fnode-sass-extra%2Fissues%2F46%3Femail_source%3Dnotifications%26email_token%3DAAKPLB35OKT2P5734XP4EWTQIREQ7A5CNFSM4ITA6JEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6FFF3Y%23issuecomment-529158895&data=02%7C01%7C%7C818e31504f8a43583bb908d733f7471f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637035009443699245&sdata=ufgFd%2B5WhdycBvbpQWYvIrPj220NKWT0Vx5wbAvQLWc%3D&reserved=0, or mute the threadhttps://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAKPLB3AUF3AV7TUEQBYTTTQIREQ7ANCNFSM4ITA6JEA&data=02%7C01%7C%7C818e31504f8a43583bb908d733f7471f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637035009443709250&sdata=hoZzx1FpMhr7eU8ehkYp8%2FiIih4wJg8b4JmQSrVfl1s%3D&reserved=0.

marksmccann commented 5 years ago

For the output, I think that you are right, but the source should always be defined. I guess this solves half the problem :). I guess for output we can just make an assumption and then document it?