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

postcss-load-config to postcss org #360

Closed
alexander-akait opened this issue May 4, 2018 · 15 comments
Closed

postcss-load-config to postcss org #360

alexander-akait opened this issue May 4, 2018 · 15 comments

Comments

@alexander-akait
Copy link
Member

@michael-ciniawsky @ai
I think we should move postcss-load-config under postcss org to better maintenance, anyone can be absent for some reason, i think better move this repo under postcss org, then update deps and fix problem.

Related issues: #358, maybe #307

@ai
Copy link
Contributor

ai commented May 4, 2018

Or even move it to the core 😏

@alexander-akait
Copy link
Member Author

@ai I think moving it to the core can take more time (triangle/discussion/other meta stuff). I agree will be great have this in core. Just to eliminate current issues, it would be enough to transfer this to repo and upgrade deps.

@alexander-akait
Copy link
Member Author

alexander-akait commented May 5, 2018

/cc @michael-ciniawsky can we update deps for postcss-load-config? cosmiconfig has better perf on 4 version

@ai
Copy link
Contributor

ai commented May 5, 2018

I don’t have npm access to help here with a quick release. Try to write an email.

@alexander-akait
Copy link
Member Author

@ai let's wait @michael-ciniawsky if no answer, we can recreate new repo 😞

@ai
Copy link
Contributor

ai commented May 5, 2018

Forking is not an option. Especially, just to update dependencies.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented May 5, 2018

We can of course move it under postcss and everyone who needs it can have any access to, since it is meant to be the common postcss config and not just some opinionated module of mine. @evilebottnawi It works without any issues atm, so their is really no need to rush anything here :). Any kind of 'perf' improvement is claiming until it is actually benchmarked... There are a few things outstanding for postcss-load-config but all changes need to be tested across the different runners and I just haven't found the time to do so yet. Global requires for the CLI and better {Error}s are needed, but I was personally waiting for a few things while there isn't any urgency, hoping that

  • node's decision on how ES Modules will work in the future and what this may mean for postcss-load-config and global/local plugin loading (unresolved). This is important since loading local plugins from a global config requires to hook into module (CJS) which may not be possible with import() (ES Modules) and may cause trouble to migrate later on.
  • Breaking Changes of cosmiconfig v5.0.0 (which was released ~2 hours ago) (resolved). We may also consider just resolving the config on our own since cosmiconfig includes extras we don't really need. Supporting postcss.config.js,.postcssrc.js && pkg.postcss with ctx should be enough. The search algo is trivial here and doesn't really require and an additional module. Still ensuring searching works the same for existing postcss.config.js etc would be needed then
  • plugins: <Array> won't work with 'global' requires, so this means that it's not just a choice of 'style', but can also result in different config behavior (bad)

Also for a new major version we might in general triage what we can do better beforehand, what breaking changes (should be avoided at all cost) this would cause etc etc..

Making it just one module would also be a good idea, since splitting it into 3 was a very bad idea of mine back then

@ai
Copy link
Contributor

ai commented May 5, 2018

@michael-ciniawsky glad you are back 😸. Yeap, no rush.

Could you give me (iskin at npm) npm access just in case of security or other critical issue?

I am still thinking what we will get from adding library as dependency to PostCSS core. Seems like right now all PostCSS runners anyway use it.

@michael-ciniawsky
Copy link
Member

It could also be included in core directly instead of being a separate module or core ships with the module included by default (with a condition to exclude it for other targets then node) trying to load a config via options.from || process.cwd(). Since it will always require access to the File System and therefore 'only' really work with node (which is the main use case), the question is if this unnecessarily complicates bundling postcss for other targets (e.g browser). Still something to explore since the implementation is small and trivial atm

Could you give me (iskin at npm) npm access just in case of security or other critical issue?

Done

@ai
Copy link
Contributor

ai commented May 5, 2018

@michael-ciniawsky there is a nice technique with browser key in package.json, which we discovered for Nano ID during Size Limit benchmarking.

We used in Browserslist to have separated parts for Node.js and browser.

@michael-ciniawsky
Copy link
Member

Yep, something like that :)

@alexander-akait
Copy link
Member Author

alexander-akait commented May 5, 2018

@michael-ciniawsky thanks for access, now we can solve security problems and big bugs 👍

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented May 5, 2018

😮 I just added @ai on npm atm. Who and what access does anyone envolved need (github/npm) ? Should we move it to postcss or leave it and discuss core integration first (since the module(s) is then deprecated and obsolete soon anyways) ?

@ai
Copy link
Contributor

ai commented May 5, 2018

I think my contact will be enough.

Yeap. Let's create a issue in postcss/postcss

@michael-ciniawsky
Copy link
Member

postcss/postcss#1147

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

No branches or pull requests

3 participants