feat: re-use postcss ast if passed from previous loader #425

jquense · 2020-02-10T16:09:12Z

Notable Changes

Allows reusing a postcss AST if the previous loader passed it. Same logic as css-loader, accepts semver compatible AST versions. It skips doing parser specific logic, since a previous AST makes parsing irrelevant

Type

ℹ️ What type of changes does your code introduce?

SemVer

ℹ️ What changes to the current semver range does your PR introduce?

Fix (:label: Patch)
Feature (:label: Minor)
Breaking Change (:label: Major)

Checklist

ℹ️ You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is a reminder of what we are going to look for before merging your code.

Lint and unit tests pass with my changes
I have added tests that prove my fix is effective/works (if needed)
I have added necessary documentation (if appropriate)
Any dependent changes are merged and published in downstream modules

jquense · 2020-02-10T16:09:53Z

test/options/exec.test.js

  test('JSS - {String}', () => {
    const config = {
      loader: {
+        test: /style\.js$/,


this test was wrong, wasn't actually testing anything

Feel free to fix it

alexander-akait

Just for information - what is use case? Why do not use postcss plugins directly?

jquense · 2020-02-10T16:37:30Z

I don't necessarily have strong use-case. Thinking a bit proactively.

you might want to do: css-loader!postcss-loader!css-module-loader for autoprefixing.

Basically the use case is loaders that use postcss but can't be simplified to just a postcss plugin.

alexander-akait · 2020-02-10T17:01:54Z

Can you fix tests?

jquense · 2020-02-10T17:07:26Z

I can update the snapshots, but I believe these were broken before I made any changes, esp since i haven't touched that code...

alexander-akait

Thanks!

jquense · 2020-05-01T18:35:46Z

anything else i can do here?

alexander-akait · 2020-05-04T12:04:18Z

test/options/sourceMap.test.js


      expect(source).toEqual(
-        'module.exports = "a { color: rgba(255, 0, 0, 1.0) }\\n\\n/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRlc3QvZml4dHVyZXMvY3NzL3N0eWxlLmNzcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQSxJQUFJLDJCQUFZLEVBQUUiLCJmaWxlIjoidGVzdC9maXh0dXJlcy9jc3Mvc3R5bGUuY3NzIiwic291cmNlc0NvbnRlbnQiOlsiYSB7IGNvbG9yOiBibGFjayB9XG4iXX0= */"'
+        'module.exports = "a { color: rgba(255, 0, 0, 1.0) }\\n\\n/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRlc3QvZml4dHVyZXMvY3NzL3N0eWxlLmNzcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQSxJQUFJLDRCQUFhIiwiZmlsZSI6InRlc3QvZml4dHVyZXMvY3NzL3N0eWxlLmNzcyIsInNvdXJjZXNDb250ZW50IjpbImEgeyBjb2xvcjogYmxhY2sgfVxuIl19 */"'


/cc @jquense It is strange, why source map was changed

i don't know, this was broken before my PR I think: #425 (comment)

@jquense maybe postcss have bugs with source maps when we share AST?

no these tests are failing on master right now

the source map difference is unrelated to this PR, my guess is it's a difference from node 10 to 12, this repo is really inactive so maybe no one has noticed until now

I think it is bug on postcss side when we share AST

I faced with same problem in css-loader and put it in my TODO for the next major release

I really don't think it's a bug with this PR, if I make NO changes to the code on a fresh checkout of postcss-loader/master these two tests fail.

Yep, let's merge, improving source maps for CSS pipeline in my TODO, so let's check and tests all in other PRs

test/loader.test.js

michael-ciniawsky

Could you please open separate PR's for the unrelated test changes? It's not a blocker for this PR to land and better to investigate them separately.

Looks good overall and worthwhile to add just a few nits and it should be good to go

michael-ciniawsky · 2020-05-14T12:33:53Z

src/index.js

@@ -1,9 +1,12 @@
 const path = require('path')

+const { satisfies } = require('semver')


I don't really want to add another dependency if possible, do we really need the 'full' semver package or would a smaller utility function also solve it e.g

const SEMVER_RE = /\^?\~?(\d+)\.(\d+).(\d+)/ function semver (version) { const [ _, major, minor, patch ] = version.match(SEMVER_RE) return { major, minor, patch } } const version = semver(postcssPkg.version) // ...

Please see the comment below for further discussion

parsing semver with a simple regex is a generally a bad idea, and can fail in unexpected ways. I get not wanting another dependency but css-loader already does this and it's probably more worthwhile to match the behavior esp since the dependency is most likely already installed

Your regexp is invalid, parsing versions using regexp is a bad idea

Why? I mean I'm simply not convinced this requires to have all blows and whistles of the semver spec to work and therefore the whole semver package as a dependecy might be avoidable, but let's not hung ourselves up on this for to long, like mentioned above I will open an issue in the postcss repo

michael-ciniawsky · 2020-05-14T12:42:17Z

src/index.js

+    meta &&
+    meta.ast &&
+    meta.ast.type === 'postcss' &&
+    satisfies(meta.ast.version, `^${postcssPkg.version}`)


What/Which format does meta.ast.version currently return? I think the major should be enough as it is unlinkely to have AST changes without introducing a BREAKING CHANCE. Needs to be documented somehow in any case.

Assumming to return the major version ist schould be sufficient to e.g

if ( meta && meta.ast && meta.ast.type === 'postcss' && meta.ast.version === version.major ) ...

I also think the meta property is always defined, but not 100% sure anymore...

it's the full version string from the postcss Processor. hopefully just the major version is enough to determine breaking changes, but ppl and computers are messy and breaking changes in AST's happen whenever in practice. Yes this a "best guess", but i think it's wise to encode more rather than less information here since we know it. It means that down the road if something does break unexpectedly and we (or consumers of this metadata downstream) need to adjust on a minor or patch version it would be possible, and not require a breaking API change here to add

I will open an issue in the postcss repo maybe it's possible to add the version to e.g the ast.root so any third party tool does have to invent it's own way to handle this (not blocking this PR on it) but maybe we are luky...

oo great idea

src/index.js

test/ast-loader.js

test/loader.test.js

michael-ciniawsky · 2020-05-14T13:41:40Z

postcss/postcss#1359

jquense · 2020-05-15T16:24:21Z

split out test changes and addressed feedback

alexander-akait · 2020-08-17T13:32:50Z

@jquense friendly ping, still valid?

jquense · 2020-08-17T13:35:40Z

still valid, I haven't had a chance to rebase tho

alexander-akait · 2020-08-17T13:38:59Z

@jquense can you provide link on loader there it can be used?

jquense · 2020-08-17T13:43:29Z

I don't have a specific case aside from like css-module-loader, the other case is another postcss-loader in front of it. I don't have a super strong justification other than "why not? css-loader does it"

alexander-akait · 2020-08-17T13:59:09Z

@jquense just asked it, because want to know what somebody use it, we will shipped it in the next major release (ETA is current week I think)

jquense · 2020-08-19T14:40:11Z

ya, it's more of an internal optimization that seemed good to do since postcss is often uses as a tooling base for other more specific loader chains

jquense requested review from alexander-akait and michael-ciniawsky as code owners February 10, 2020 16:09

jquense commented Feb 10, 2020

View reviewed changes

alexander-akait reviewed Feb 10, 2020

View reviewed changes

alexander-akait approved these changes Feb 10, 2020

View reviewed changes

alexander-akait reviewed May 4, 2020

View reviewed changes

jquense commented May 6, 2020

View reviewed changes

test/loader.test.js Outdated Show resolved Hide resolved

alexander-akait approved these changes May 13, 2020

View reviewed changes

jquense mentioned this pull request May 13, 2020

Custom loader #428

Closed

17 tasks

michael-ciniawsky suggested changes May 14, 2020

View reviewed changes

michael-ciniawsky added priority: medium scope: postcss semver: minor status: pending type: feature labels May 14, 2020

michael-ciniawsky added this to the 3.1.0 milestone May 14, 2020

michael-ciniawsky self-assigned this May 14, 2020

michael-ciniawsky mentioned this pull request May 14, 2020

Add the current version to the AST (Root) postcss/postcss#1359

Closed

jquense force-pushed the reuse-ast branch from 0ea19d0 to 77e2218 Compare May 15, 2020 16:20

feat: re-use postcss ast if passed from previous loader

05cdfda

jquense force-pushed the reuse-ast branch from 77e2218 to 05cdfda Compare May 15, 2020 16:22

jquense mentioned this pull request Aug 3, 2020

test: fix incorrect test #430

Closed

14 tasks

alexander-akait mentioned this pull request Sep 7, 2020

feat: reuse AST from other loaders #468

Merged

6 tasks

alexander-akait closed this in #468 Sep 7, 2020

		@@ -1,9 +1,12 @@
		const path = require('path')

		const { satisfies } = require('semver')

Uh oh!

feat: re-use postcss ast if passed from previous loader #425

feat: re-use postcss ast if passed from previous loader #425

Uh oh!

Conversation

jquense commented Feb 10, 2020

Notable Changes

Type

SemVer

Checklist

Uh oh!

jquense Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

jquense commented Feb 10, 2020

Uh oh!

alexander-akait commented Feb 10, 2020

Uh oh!

jquense commented Feb 10, 2020

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

jquense commented May 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jquense Feb 10, 2020 •

edited

Loading

alexander-akait May 14, 2020 •

edited

Loading

michael-ciniawsky May 14, 2020 •

edited

Loading