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

Detect files added to/removed from directories. #2615

Merged
merged 10 commits into from Feb 13, 2019

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Feb 2, 2019

↪️ Pull Request

Currently, when you start parcel with glob like ./src/*, parcel watches every change in src directory. But when we add a new file like good-button.scss to src directory, parcel cannot detect it.

To do that, we need to restart parcel manually. This PR fixes that problem.

It closes old issues like #667, #2211.

💻 Examples

  1. Start parcel with glob like ./src/*, or ./src/*.js
  2. Add/Remove a new file that matches that glob.
  3. parcel creates new bundle.

🚨 Test instructions

There are 4 test cases.

  • glob match, a new file added -> rebuild
  • glob match, a file removed -> rebuild
  • glob not match, new file added -> don't rebuild
  • glob not match, a file removed -> don't rebuild

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

@sainthkh sainthkh changed the title Detect new files added to/removed from directories. Detect files added to/removed from directories. Feb 2, 2019
@chrisdmacrae
Copy link

@DeMoorJasper @sainthkh what's the state of this PR?

If I can jump in and contribute, I will. This fix is extremely important for a tool I'm building with Parcel! 👍

@DeMoorJasper
Copy link
Member

@chrisdmacrae tests are failing so it won't get merged untill that's fixed.

There was a similar PR to this here: #2211 that had every test passing, so might be an inspiration for improving this. But I think it's just a simple timeout in the tests

@sainthkh
Copy link
Contributor Author

No one reviewed code. So I simply abandoned it and wait for opinions.

I guessed that the parcel team is too busy to check this PR.

@sainthkh
Copy link
Contributor Author

When they show interest, I'm ready to fix the tests.

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Please fix the failing tests

@sainthkh
Copy link
Contributor Author

sainthkh commented Feb 11, 2019

I used old util.js because new test-util module wasn't available when I was starting this PR.

Now, that the most of the code inside util.js went to test-util module. So, I had to change the require part.

EDIT: We're all green again!

Copy link
Member

@DeMoorJasper DeMoorJasper left a comment

Choose a reason for hiding this comment

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

Could you expand this to also add watch support for glob imports?

@sainthkh
Copy link
Contributor Author

sainthkh commented Feb 12, 2019

@DeMoorJasper You mean things like require("./*.js") right?

@DeMoorJasper
Copy link
Member

@sainthkh yes

@sainthkh
Copy link
Contributor Author

Before we make glob require/import, we should decide how parcel should behave in some situations.

TL;DR; Isn't glob watching in require and import over-engineering? It can confuse users, create hard-to-find bugs and make unnecessary hiccups in our favorite tools.

1. Duplicate names.

Let's think about this file tree and codes.

src/
- a.js
- b.js
- c.js
// a.js
exports.add = function (a, b) {
  return a + b;
}
// b.js
exports.add = function (a, b, c) {
  return a + b + c;
}
// c.js

var { add } = require('./*.js'); // or
import { add } from './*.js';

// ... code that uses add function.

In c.js, what is this add? The one with 2 arguments in a.js or 3 arguments in b.js? What should we do when we need both?

2. Cannot get Visual Studio Code syntax assist.

Glob file names like *.js are not concrete. So, VS Code cannot find them. And we cannot get suggestions when our mouse pointer is hovering over names imported with globs.

We may need to write some code for VS Code plugin for this.

3. It's an error in TypeScript.

When we write code like below: VS Code will show us a red line below './*.ts'.

import { Local } from './*.ts';

export function count() {
  let local = new Local(1, 2);
  return local.a + local.b;
}

Because compiler cannot find './*.ts' file. The only solution is to turn off VS Code typescript plugin. That's too much.

4. Cyclic dependency.

Let's think about this simple file tree.

src/
- a.js
- b.js
// a.js
var { f } = require('./*.js');

exports.g = function() { 
  f();
  console.log("do something"); 
}
// b.js
var { g } = require('./*.js');

exports.g = function() { 
  g();
  console.log("do something"); 
}

In this case, it's obvious. But when we have dozens of files and we aren't careful, we may meet this problem often.

5. What should we do when a glob is removed?

Let's say a user removed require('./*.js') from a file. What should we do? We need to remove related files from chokidar. But it's a bit hard.

We might need to create a hash table of files that have glob imports and check these globs are safely there and when the contents of those files are changed. And some globs are removed, we need to loop through all the files related with this glob and remove them from chokidar.

The things will become more complicated when a file that imports a glob imports a file or files that import another glob or globs.

If we don't handle them carefully, then parcel will create weird bundle.

Conclusion: Do we really need it?

I made this feature because we sometimes need to compile some files when it is not required or imported in the descendants of the index.js, style.css file.

For example:

  • when you use css modules and typescript, you need to automatically create d.ts files even if the css (or scss or less) file isn't included in any ts file. If not, it'll cause errors when you first compile it. (It's the famous problem for this loader.)
  • when you're a ReasonML developer, some files won't get compiled when they're changed because ReasonML doesn't have things like require/import.

We can solve these problems by starting parcel like parcel *.re *.scss. (Actually, it's what I wanted and why started this PR.)

And I guess that the vue file problems in #667 can be solved with this command line glob like parcel index.js *.vue.

With these reasons, I think adding glob watching is more harmful than useful.

Or am I missing some crucial use cases? I want your opinions.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented Feb 13, 2019

@sainthkh you’re right and i’m not sure if globs will remain in Parcel 2. That one edge case should never happen as it would import c.js again and potentially throw errors

Sent with GitHawk

@sainthkh
Copy link
Contributor Author

Then, we can say that this PR is done. 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.

None yet

3 participants