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

Extends and standardize dependency message #1537

Closed
ai opened this issue Mar 7, 2021 · 26 comments
Closed

Extends and standardize dependency message #1537

ai opened this issue Mar 7, 2021 · 26 comments
Labels

Comments

@ai
Copy link
Member

ai commented Mar 7, 2021

result.messages.push({ type: 'dependency', file }) is a special API for plugin developers to tell about used files to PostCSS runner. The most popular way to use it is a watch mode.

But seems like we need to improve this message:

  1. Just the file path is not enough. For instance, postcss-mixins accept glob pattern.
  2. Seems like webpack’s css-loader needs more types: missing, build, context and regular dependencies. I do not like separated types like build-dependency. I think it will be better to use { type: 'dependency', build: true }

Questions:

  1. @mischnic @RyanZim Can every runner support glob-pattern for file watch?
  2. @alexander-akait can you explain what is build/context/missed dependency? It maybe a good idea to use these messages in other builders too.
@ai ai added the question label Mar 7, 2021
@mischnic
Copy link
Contributor

mischnic commented Mar 7, 2021

Parcel 2 has APIs for:

  1. watching individual files for modification and deletion
  2. watching for file creations of individual files, globs and also for when a file of a specific name is created in a parent folder of a specific file (e.g. .babelrc is created in some parent folder of a JS file)

I couldn't figure out what { type: 'dependency', build: true } would mean here, but maybe that already covered by Parcel's distinction between creation and modification+deletion

@adamwathan
Copy link
Contributor

Chiming in for Tailwind CSS — for some stuff we are working on it is super helpful if we have a way to register preferably globs but at least directories as dependencies, as we need to re-build when new files are created that live in certain directories.

Perfect world for us is being able to do this and be notified any time any matches change, are deleted, or new files are added that match the glob:

result.messages.push({ type: 'dependency', file: './src/**/*.{js,html}' })

I know postcss-cli already supports this, but I suspect not really deliberately, as the paths are just passed to chokidar which does support globs.

As far as I know webpack does not support any type of glob dependency, but you can register files or directories, which means if you can parse globs in postcss-loader then you can simulate it closely enough.

I believe Vite only supports files currently, not directories or globs, but I don't know enough about the internals to know if support directories would be meaningfully simpler than supporting globs.

I think in an ideal world we would be able to push out globs from within PostCSS and build tools would support that, but it's also important to be practical so if that is extremely complex or impossible in any of the popular build tools it might be a good idea to settle for files and directories.

@adamwathan
Copy link
Contributor

I spoke with @yyx990803 about what would/wouldn't be doable in Vite and he confirmed that supporting globs is not a problem at all, he just doesn't want to invest in it unless it's standard and expected behavior, which is why I think it's good we are having this discussion to formalize the API from PostCSS's perspective 👍🏻

@RyanZim
Copy link
Contributor

RyanZim commented Mar 8, 2021

As @adamwathan pointed out, postcss-cli does support globs (admittedly by accident, but we can make that explicit).

However, worth noting that postcss-cli currently only watches for file changes and doesn't even watch for new files that fall under the input glob (let alone dependencies). The issue is postcss/postcss-cli#161; it'll be fairly involved to add, and I just don't have the bandwidth to do it myself. Happy to review PRs if someone else steps up for it, though.

@RyanZim
Copy link
Contributor

RyanZim commented Mar 8, 2021

Also not understanding what you have in mind with a build parameter.

@alexander-akait
Copy link

alexander-akait commented Mar 9, 2021

Supporting globs is not hard, but it definitely degrades performance.

@adamwathan Can you describe why you need watch globs? Because when you do glob you known which files/directories you are use...

Seems like webpack’s css-loader needs more types: missing, build, context and regular dependencies. I do not like separated types like build-dependency. I think it will be better to use { type: 'dependency', build: true }

It's all about performance. build-dependency aka configurations files like postcss.config.js need to invalidate cache, because not every change can invalidate module, each postcss-plugin can have own complex logic, for example generation of sprites, adding new svg can invalidate the whole module and can nothing do (because it can be filtered inside plugin, and nobody known about it expect own plugin). I can describe more cases and more complex cases.

Another problem with glob is that it is not standardized, unfortunately, different systems work differently, even different packages work different, and nobody can say what is right.

@adamwathan
Copy link
Contributor

@alexander-akait Yeah I can explain! We are building a new version of Tailwind that generates CSS on demand based on the contents of your template files (React components, HTML files, Vue files, whatever you are using), similar to how the ACSS atomizer works.

To do this, the end user needs to configure Tailwind with a list of globs pointing to all of their template files, which looks something like this:

// tailwind.config.js
module.exports = {
  files: [
    './components/**/*.{js,jsx,ts,tsx}',
    './pages/**/*.{js,jsx,ts,tsx}',
    './public/**/*.html',
  ]
  // ...
}

We can parse these globs and register the files directly ourselves but then we can't rebuild when new files are added that match the globs. Being able to register a directory as a dependency would probably be good enough, although it would mean rebuilding more frequently than necessary because someone could add/change a file in a directory but with a different file extension and trigger a rebuild. For example, if our glob is only looking for js files but someone adds a yaml file to that directory, it would cause an unnecessary rebuild.

Ultimately I think being able to register files or directories but not globs would still be good enough for us. I wonder though if maybe it could be the job of postcss-loader to translate a glob into a set of files and directories instead, so from a PostCSS plugin author's perspective they still only need to be concerned with globs?

Again though we can work with files + directories, it would just mean parsing the glob internally in Tailwind and turning that into a list of files and directories to register.

Our workarounds for all of this currently is to run our own chokidar watcher and create a file in ~/.tailwindcss that we register as a PostCSS dependency and touch whenever we need to imperatively trigger a rebuild. Getting this to play nicely with webpack 5's persistent file cache has been very challenging as well.

@alexander-akait
Copy link

We can parse these globs and register the files directly ourselves but then we can't rebuild when new files are added that match the globs. Being able to register a directory as a dependency would probably be good enough, although it would mean rebuilding more frequently than necessary because someone could add/change a file in a directory but with a different file extension and trigger a rebuild. For example, if our glob is only looking for js files but someone adds a yaml file to that directory, it would cause an unnecessary rebuild.

Glob support won't solve this problem, on the contrary, it degrades performance, because when you will evaluate glob, you will do a lot of fs calls, after you passed glob dependency for us, we will need to do it too on own side, so on each change you and we will need to a lot of fs calls. You shouldn't be afraid to pass context-dependency in this case.

Ultimately I think being able to register files or directories but not globs would still be good enough for us. I wonder though if maybe it could be the job of postcss-loader to translate a glob into a set of files and directories instead, so from a PostCSS plugin author's perspective they still only need to be concerned with globs?

Yes we can do it, but as I said earlier this is not standardized and this greatly reduces performance (a lot of fs calls), and you already know this set of files, why not just give them to us.

As I see it from my side:

  1. On first run you evaluate glob and pass dependency (file) and context-dependency (directory)
  2. When developer changes something in dependency or context-dependency, we run postcss-loader again
  3. tailwind has Map and checks which files were change and rebuild or do nothing, even more it can be persistent cache so next builds (not in watch mode) will be faster

Pros of this solution - bundle agnostic, built-in CLI (for example postcss-cli) in watch mode will be fast (with persistent cache for separated builds too), no extra fs calls. We have the same logic for copy-webpack-plugin (we use globby inside), here we check snapshot for file https://github.com/webpack-contrib/copy-webpack-plugin/blob/master/src/index.js#L375 (i.e. timestamp and content and some meta information, we have more complex logic here), if snapshot valid (i.e. timestamp and content are the same) we don't do extra fs call and transformations, if invalid we do fs call and transformations https://github.com/webpack-contrib/copy-webpack-plugin/blob/master/src/index.js#L396. Anyway you need to run globs every time, this has always been a problem when using globs, somebody should evaluate it to get list of files and it is require time...

What can be snapshot for file - { pathToFile: { source: originalSourceOfFile, classes: ['foo', 'bar', 'baz'] } }. Anyway it is just idea.

The main problem is that we don't know which files require to rebuild files, and glob doesn't solve it, because we still need to look at whole directory here and do a lot of extra jobs, it is not solve the problem, patterns can be very complex... Only tailwind plugin known it. I don't think messages API can solve it.

Other idea - you can get modified files using loaderContext._compiler.modifiedFiles https://github.com/webpack-contrib/postcss-loader#function (here also removedFiles, fileTimestamps and contextTimestamps, but it is bundle specific).

@ai
Copy link
Member Author

ai commented Mar 11, 2021

What do you think about these changes?

Standard file dependency:

{ type: 'dependency', plugin, parent, file }

Directory:

{ type: 'dependency', plugin, parent, dir }
// For compatibility we can add `file` prop as well { …, file, dir }
// We can use `type: 'dirDependency'` if we want a different type,
// but 'context-dependency' is not very clear name

Missed:

{ type: 'dependency', plugin, parent, file, missed: true }

“Build” dependency:

{ type: 'dependency', plugin, parent, file, invalidateAll: true }

@alexander-akait
Copy link

alexander-akait commented Mar 11, 2021

It is more misleading than we have, invalidateAll (build-dependency) doesn't mean module will be invalidated...

Why we need plugin here? And what is parent?

In most time developer need only dependency, it is very rare case when postcss plugin have own config file (i.e build-dependencies), watching directories and missing files (look parcel have the same API invalidateOnFileCreate) are also rare.

Anyway I think using type is better than options like file/dir.

Also watching files and directories have always different API, for example chokidar:

const watcher = chokidar.watch('file, dir, glob, or array', {
  ignored: /(^|[\/\\])\../, // ignore dotfiles
  persistent: true
});

watcher
  .on('add', path => log(`File ${path} has been added`))
  .on('change', path => log(`File ${path} has been changed`))
  .on('unlink', path => log(`File ${path} has been removed`));

// More possible events.
watcher
  .on('addDir', path => log(`Directory ${path} has been added`))
  .on('unlinkDir', path => log(`Directory ${path} has been removed`))

So having dependency and context-dependency make sense (if it is misleading, we can rename them to file-dependency and dir-dependency), build-dependency is also very simple to describe - it is any configurations files, only missing is not easy to understand...

@bradlc
Copy link
Contributor

bradlc commented Mar 11, 2021

Hey everyone, wanted to pitch in here on behalf of Tailwind CSS also.

"Build" dependencies: As I understand it these are dependencies that invalidate the entire build. I agree with @alexander-akait that it is very rare that you would need this in a PostCSS plugin, and this is not something that we would require for Tailwind CSS. I am also concerned that not all bundlers could support this type of dependency currently. For example could this be supported by rollup-plugin-postcss or Parcel (@mischnic)?

"Missing" dependencies: Again this is probably rare and not something that we would require. However, we could potentially utilise missing dependencies to optimise rebuilds. Again though, my concern is whether all bundlers understand the concept of "missing" dependencies. Parcel does via invalidateOnFileCreate as @mischnic and @alexander-akait have already mentioned, but what about Rollup and others?

I think that it should be a priority to ensure that all tools/bundlers are able to support any new messages. For that reason I would like to propose that (at least initially) we standardise a way to register directory dependencies only, as a new type and/or a new property:

{ type: 'dependency', directory: '/my-project' }

// or

{ type: 'directory-dependency', directory: '/my-project' }

Personally I think a new type is a better approach as it maintains the current situation of one dependency per message. With the first example you would be able to specify a file and a directory in the same message which feels a bit weird to me:

{ type: 'dependency', directory: '/my-project', file: '/random-file.txt' }

Alternatively, if "build" and "missing" dependencies are desirable then I think we need to verify that tools can comfortably support them.

What do you think?

@alexander-akait
Copy link

Looks rollup have https://rollupjs.org/guide/en/#thisaddwatchfileid-string--void, i.e. dependency and context-dependency and missing-dependency (for files), rollup doesn't support cache so, build-dependency doesn't have sense. So most of bundlers have API

@mischnic
Copy link
Contributor

invalidate the entire build

I don't understand what this means. If some file depends on the contents another file, then changing the second file would make Parcel rebuild the first. There shouldn't be any global state that somehow invalidates "everything". (The only similar mechanism is for JS config files, where the actual returned config isn't necessarily deterministic. So this causes some files to always be rebuilt on one-shot builds, but this mechanism is inherently incompatible with watch mode).

Glob support won't solve this problem, on the contrary, it degrades performance

I want to second that. With many globs in the project, every file change event needs to checked against every glob, so the time for rebuilds now also depends on the size of the project, instead of just on the size of changes.

directory

At least the moment, Parcel doesn't have an API for listening on file creations inside of a specific directory apart from globs. (But this would certainly be better for performance than globs.)

@ai
Copy link
Member Author

ai commented Mar 11, 2021

Why we need plugin here?

For debug. In a real system, it could be 20+ plugins.

And what is parent?

File, where this dependency was found. For instance, CSS file with url(file.png). https://postcss.org/api/#result-messages

We standardise a way to register directory dependencies only, as a new type and/or a new property:

Yeap. We need to support the old API. I like type: 'dependency', file and type: 'dirDependency', dir

"Missing" dependencies: Again this is probably rare and not something that we would require.

Runners should support each message feature only if they can improve DX by this message. If you do not know what to do with missing dependency, you can ignore it.

build-dependency is also very simple to describe - it is any configurations files

There is a good idea, that we should describe “how we will process something” instead of popular use case.

What do you think about invalidateCaches?

only missing is not easy to understand...

AT least for me, it is clear. A dependency for the file, which was not created yet.

@alexander-akait
Copy link

What do you think about invalidateCaches?

I found invalidateCaches name is misleading, to be honestly it is not mean invalidate cache, it mean current module depend on this file as build dep, in theory plugin can implement more complex logic and decide what to do. Most of postcss plugins doesn't have cache (memory/persistent), so most of times they will be whole rebuild. To be honestly build name is very simple and understandable.

@ai
Copy link
Member Author

ai commented Mar 13, 2021

OK. So the final design:

{ type: 'dependency', file, build: true, missing: true }
{ type: 'dirDependency', dir }

Does it look good?

@alexander-akait
Copy link

I think for string better to use dir-dependency, maybe for dependency use file-dependency to avoid small misleading

@ai
Copy link
Member Author

ai commented Mar 13, 2021

I think for string better to use dir-dependency

OK. It will be more CSS-ish.

maybe for dependency use file-dependency to avoid small misleading

We need compatibility with old API

@bradlc
Copy link
Contributor

bradlc commented Mar 16, 2021

Runners should support each message feature only if they can improve DX by this message. If you do not know what to do with missing dependency, you can ignore it.

My concern with this is that if some dependency types are not supported by certain runners then they become unusable. If I create a PostCSS plugin using a "build" dependency and Rollup (for example) doesn't support it then my plugin won't work correctly with that runner.

If you want to add these other dependency types perhaps we need to standardize a way for runners to declare which dependency types they support? Plugins can use this information to, for example:

  1. Show an informative error explaining that the plugin is incompatible with this runner because it doesn't support the necessary dependency types.
  2. Avoid using unsupported dependency types. For example instead of using a "missing" dependency you may be able to use a "directory" dependency instead. But in order to do this we need to know when a runner doesn't support "missing" dependencies. Otherwise we will just register the dependency as "missing" and the runner will ignore it, therefore silently breaking the plugin.

Here's an example:

const postcss = require('postcss')

postcss([
  function (root, result) {
    let supportedDependencyTypes = result.opts.dependencyTypes || ['dependency']

    if (supportedDependencyTypes.includes('missing-dependency')) {
      result.messages.push({ type: 'missing-dependency', /* ... */ })
    } else if (supportedDependencyTypes.includes('dir-dependency')) {
      result.messages.push({ type: 'dir-dependency', /* ... */ })
    } else {
      throw Error('Runner does not support directory dependencies.')
    }

    return root
  },
])
  // runner adds the list of dependency types it supports here
  .process('/* ... */', { from: undefined, dependencyTypes: ['dependency', 'dir-dependency'] })
  .then((result) => {
    /* ... */
  })

@ai
Copy link
Member Author

ai commented Mar 16, 2021

@bradlc I think it will be completely OK if the runner without build dependency support will treat it as a simple dependency.

Some messages could use advanced optimizations inside some specific bundler. Another bundler may have unique architecture and messages that will be relevant only for this architecture and can’t be used on different runners.

@bradlc
Copy link
Contributor

bradlc commented Mar 17, 2021

I think it will be completely OK if the runner without build dependency support will treat it as a simple dependency.

I don't really agree with this. If I register a dependency as a build dependency then I expect it to behave like a build dependency. If it doesn't and just silently behaves like a normal dependency then that could lead to unexpected behaviour and bugs.

@ai
Copy link
Member Author

ai commented Mar 21, 2021

I don't really agree with this. If I register a dependency as a build dependency then I expect it to behave like a build dependency.

It could be a runner, who does not have caches at all and always build everything from the scratch.

@bradlc
Copy link
Contributor

bradlc commented May 12, 2021

Hey @ai, would love to keep the discussion going on this, with a view to getting something finalised and documented.

How would you feel about initially adding documentation only for a new dir-dependency message type? This would look like this as previously discussed:

{
  type: 'dir-dependency',
  dir: '/example'
}

Other types could be added in future but this is certainly the one that will be most useful for plugin authors, and enable new use-cases.

Additionally, I still believe that runners should have a way to declare to plugins which message types they support. This will allow plugins to make informed decisions about which messages to use, or throw an error when a required message type is not supported. This is critical for developer experience.

For example, we would love to use directory dependencies within the Tailwind CSS plugin to enable robust support for our JIT engine across all runners. We currently use workarounds for this, with varying levels of success. Ideally we would be able to do something like this:

if (runnerSupportsDirDependencies) {
  // use dir dependencies 👍
} else {
  // use old workaround method, which still works in most cases 👍
}

This would allow us to migrate to using directory dependencies in a seamless way that ensures that our plugin still works even with runners that have not yet implemented support for the new message type.

@ai
Copy link
Member Author

ai commented May 13, 2021

Yeap, let’s add dir-dependency to docs for now with dir key. Please send PR.

@ai
Copy link
Member Author

ai commented May 13, 2021

Dir dependency was added to plugin and runner guidelines

#1580

@ai ai closed this as completed May 13, 2021
@ai
Copy link
Member Author

ai commented Jun 2, 2021

We added optional glob parameter to dir-dependnecy: { type: 'dir-dependency', dir: '/imported', glob: '**/*.css', }

#1590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants