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 support for PostCSS dir-dependency messages #6299

Merged
merged 11 commits into from Jun 3, 2021

Conversation

bradlc
Copy link
Contributor

@bradlc bradlc commented May 17, 2021

↪️ Pull Request

This PR updates the PostCSS transformer, adding support for dir-dependency messages specified by PostCSS plugins (documentation). The implementation is very similar to part of the Stylus transformer.

💻 Examples

Example project with instructions in readme: https://github.com/bradlc/parcel-dir-dependency

🚨 Test instructions

I can't see any tests for the existing dependency messages, but please let me know if I can do anything in this regard.

@height
Copy link

height bot commented May 17, 2021

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@devongovett
Copy link
Member

Thanks. Do you think we should also support a glob message type? I could see typically needing to filter the files in some way rather than just getting everything. For example, for Tailwind you might need only .html files or whatever you've configured as your templates. Including all files would probably slow things down and cause unnecessary cache invalidations.

@bradlc
Copy link
Contributor Author

bradlc commented May 17, 2021

Hey @devongovett! I think there were a couple reasons that directory dependencies were prioritised over globs:

For the first point, I could see PostCSS documenting an additional glob-dependency type, which runners could optionally implement if they are able to. I think in this case runners would need to declare which dependency types they support so that plugins can make an informed decision about which dependencies to register, for example:

let pattern = '/src/**/*.html'

if (supportsGlobDependencies) {
  messages.push({ type: 'glob-dependency', glob: pattern })
} else if (supportsDirectoryDependencies) {
  messages.push({ type: 'dir-dependency', dir: getBase(pattern) })
} else {
  throw Error('Runner does not support glob or directory dependencies.')
}

(I actually like the idea of runners declaring the types they support even with just dependency and dir-dependency, and would like to see this standardised in the PostCSS docs, but that's a separate issue.)

The second point is trickier though, I think. What if one runner uses glob and another uses fast-glob? Depending on the patterns you use behaviour could differ across runners. Does that matter? I'm not sure.

@devongovett
Copy link
Member

Yeah a way to indicate which dependency types are supported sounds like a great idea. As for what glob syntax should be supported, I guess I'd just keep it simple. 99% of globs will just be **/*.html or something (i.e. standard Bash globs). I'd just go with that as a starting point and see where it takes us.

@bradlc
Copy link
Contributor Author

bradlc commented May 27, 2021

Hey @devongovett. I think you've seen my pull request over on the PostCSS repository. There are some (I think valid) concerns about runners that don't support dependencies at all (because they don't need to).

Would you consider adding support (via this PR) for dir-dependency to Parcel in the meantime? We are keen for Tailwind's JIT mode to work with Parcel, and this would enable that – in the canary release setting the TAILWIND_DISABLE_TOUCH environment variable to true when running Tailwind switches to a mode which uses dir-dependency, and we plan on making this the default mode soon.

@DeMoorJasper
Copy link
Member

DeMoorJasper commented May 30, 2021

@bradlc Maybe you could merge the glob and dir dependency message into a singular dir-dependency message? This would probably simplify the logic quite a bit and not require any type of registering which dependency types are supported:

Currently

{ "type": "dir-dependency", "dir": "/src/" }

with glob (glob is relative to dir) - the glob would be optional so plugins can choose to add it or leave it out

{ "type": "dir-dependency", "dir": "/src/", "glob": "**/*.html" }

@bradlc
Copy link
Contributor Author

bradlc commented Jun 1, 2021

@DeMoorJasper I like it! 🙌 I have opened a new pull request over on the PostCSS repo for this

@bradlc
Copy link
Contributor Author

bradlc commented Jun 1, 2021

@devongovett @DeMoorJasper That PR was merged so I have updated this one to read the optional glob property 👍

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.

Awesome, thanks for adding this and getting the glob property into postcss

@devongovett
Copy link
Member

Thanks @bradlc! Added an integration test. Hope you don't mind me pushing to your branch. 😄

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