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

Add basic glob support for assets #87

Merged
merged 2 commits into from
Aug 16, 2019

Conversation

calebdwilliams
Copy link
Contributor

@calebdwilliams calebdwilliams commented Aug 16, 2019

This PR adds basic support for glob matching using explicit mode per the conversation in #85. This feature uses the npm package, glob,'s glob-matching syntax.

Example

In package.json

{
  "@pika/web": {
    "webDependencies": [
      "bootstrap/dist/css/*.css"
    ]
  }
}

Would yield the output

✔ @pika/web installed: bootstrap/dist
/css/bootstrap-grid.css, bootstrap/dist/css/bootstrap-grid.min.css, bootstrap/dist/css/bootstrap-reboot.css, bootstrap/dist/css/bootstrap-reboot.min.css, bootstrap/dist/css/bootstrap.css, bootstrap/dist/css/bootstrap.min.css. [1.59s]

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Awesome! I think this makes a lot of sense as the place to handle this logic. Just left 1 small comment about flat().

src/index.ts Outdated
const globResults = globDependencies
.map(dep => path.join(cwd, 'node_modules', dep))
.map(dep => glob.sync(dep))
.flat()
Copy link
Owner

Choose a reason for hiding this comment

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

unfortunately we still need to support Node 8+, and I believe this was introduced in Node 11.

Here's another way you could write this, half getting rid of flat & half just doing some code golf. The nice thing about using a Set is that now that we get deduplication for free (more important now that we have globs). But feel free to use this however you see fit.

const depsList = new Set();
arrayOfDeps.forEach(dep => {
  if (!glob.hasMagic(dep)) {
    depsList.add(dep);
    return;
  }
  const globLoc = path.join(cwd, 'node_modules', dep);
  glob.sync(globLoc).forEach((globMatchLoc) => {
    const globMatch = path.relative(`${cwd}/node_modules`, globMatch);
    depsList.add(globMatch);
  });
});
// Then, just replace the 1-2 later references to arrayOfDeps with the new depsList

@calebdwilliams
Copy link
Contributor Author

@FredKSchott Fair enough. I had written almost exactly what you posted at first, but the array syntax felt much more readable, so I went with that. Forgot Array.prototype.flat() is still fairly new.

@FredKSchott FredKSchott merged commit 5001dce into FredKSchott:master Aug 16, 2019
@pika-ci
Copy link

pika-ci bot commented Aug 16, 2019

🚀 This PR has been merged! Once a new release is created, it will become available on npm. Until then, you can load and install it directly from the Pika CDN:

npm install https://cdn.pika.dev/@pika/web/master

@FredKSchott
Copy link
Owner

FredKSchott commented Aug 16, 2019

Awesome! I'll get a new version cut and released, thanks for tackling!

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

2 participants