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 dependency message #233

Closed
ai opened this issue Oct 8, 2016 · 20 comments
Closed

Add dependency message #233

ai opened this issue Oct 8, 2016 · 20 comments
Assignees

Comments

@ai
Copy link
Member

ai commented Oct 8, 2016

New postcss-loader will support dependency message to notify webpack about imports. So we need to add few lines on every import:

result.messages.push({
  type: 'dependency',
  file: absoluteFilePath
})

@MoOx @TrySound write me if you need any help with it.

@ai
Copy link
Member Author

ai commented Oct 18, 2016

Any updates? How I can help with it?

@MoOx
Copy link
Contributor

MoOx commented Oct 18, 2016

First thing to do is to start again with v7, since nobody helped on the huge concurrency issue related to v8.
Codebase of v8 is far more complicated than v7 (and more buggy).
I don't have lot of time for this - I don't even use postcss-import anymore so...

People said they wanted to help (see #210) but I didn't see any PR :)

Maybe I can handle the revert to v7...

@longlho
Copy link

longlho commented Oct 18, 2016

Yes it'd be great to switch back to v7 and narrow down the feature set a bit (like import in media query). We do use this at Yahoo and I can maybe offer some contribution. (I've been PR-ing to css-modules & css-loader).

@ai
Copy link
Member Author

ai commented Oct 18, 2016

@MoOx you should give a direct order to people who want to help ;). Like “Could you fix this issue? What is ETA?”

@ai
Copy link
Member Author

ai commented Oct 18, 2016

So you need a PR for this issue? In master?

@michael-ciniawsky
Copy link

It would work the same way for CLI usage aswell

result.messages
  .filter((msg) => msg.type === 'dependency' ? msg : '')
  .forEach((dep) => chokidar.add(dep.file))

@ai
Copy link
Member Author

ai commented Oct 19, 2016

@MoOx so what help do you need to fix this issue? PR to master?

@troutowicz
Copy link

Had an issue today with HMR not working when using postcss-import 8.1.0 with postcss-loader 1.0.0. The fix suggested by @ai in webpack-contrib/postcss-loader#122 fixed my problem. Looks like the issue discussed here might be the cause?

@michael-ciniawsky
Copy link

When someone familiar with the codebase can provide a short intro to what to

  • is the issue(s) between v7/v8
  • how does postcss-smart-import and other solutions handle dependencies atm
  • is there a possiblity/interest to 'merge' postcss-smart-import and friends i the future, generally speaking, try to focus on one solution to handle @import s

I, like mentioned in other issue before, can start working on it , but starting point, where to /wht first at best would be helpful and highly appreciated :)

@MoOx
Copy link
Contributor

MoOx commented Oct 24, 2016

See #210 (comment)

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 28, 2016

@MoOx @ai Would one of you please tell me what is needs done to get this implemented? I would be willing to submit a PR, but I need to know if master is the correct place or if we are reverting to v7 first. ❗

@michael-ciniawsky
Copy link

michael-ciniawsky commented Oct 28, 2016

@RyanZim I'm neither @ai nor @MoOx but I( think it depends on your time and if your are familiar enough with the v8 code base to fix v8 related issues. If you not already aware, there is discussion going on here aswell. TLDR; postcss-smart-import, which is a basically a fork, working, updated and cleaned up , would be a good starting point for the beginning

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 28, 2016

I'm strapped for time ATM, but wouldn't mind helping/maintaining postcss-import. I noticed that issue soon after I posted this, will follow that discussion.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 4, 2016

@ai For postcss-cli, we need:

result.messages.push({
  type: 'dependency',
  file: absoluteFilePath,
  parent: fileContainingTheImport
})

Does that sound good to you?

@ai
Copy link
Member Author

ai commented Nov 4, 2016

@RyanZim sounds good (only fix a dependecy to dependency).

Do you need to add parent field to postcss-assets and postcss-inline-svg too?

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 5, 2016

Not ATM.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 5, 2016

@ai Then again, that might be a good idea to add parent to other plugins. No rush, though.

Perhaps we should make a dependency message spec someplace, so that all plugins implement it the same? Just a crazy idea...

@ai
Copy link
Member Author

ai commented Nov 5, 2016

@RyanZim we could add it to Plugin and Runner guideline. But first I want to stabilize it :D.

@ai
Copy link
Member Author

ai commented Nov 5, 2016

@RyanZim I create a issues in other plugins about parent key.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 9, 2016

PR done: #241

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

No branches or pull requests

6 participants