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

feat: support from global for composes #669

Merged
merged 4 commits into from Sep 15, 2019

Conversation

@jquense
Copy link
Contributor

commented Sep 13, 2019

This is nice feature in css-modules, for doing quick composition of global classes. global acts as a keyword instead of a file here

Description

Parses composes: a, b, c from global as composes: global(a), global(b), global(c)

Motivation and Context

Css-modules supports this, and it is nicer syntax when working with lots of global classes, such with bootstrap utility classes

How Has This Been Tested?

added a test. I could get the current tests to pass locally tho, perhaps i'm setting the repo up wrong? did lerna bootstrap in the root, assuming that was enough.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
This is nice feature in css-modules, for doing quick composition of global classes. `global` acts as a keyword instead of a file here
@TravisBuddy

This comment has been minimized.

Copy link

commented Sep 13, 2019

Travis tests have failed

Hey @jquense,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

Node.js: 8

View build log

npm test -- --ci
> modular-css@0.0.0 pretest /home/travis/build/tivac/modular-css
> npm run parsers


> modular-css@0.0.0 parsers /home/travis/build/tivac/modular-css
> node parsers/build.js


> modular-css@0.0.0 test /home/travis/build/tivac/modular-css
> jest "--ci"

 PASS   tests  packages/rollup/test/rollup.test.js
 PASS   tests  packages/processor/test/options.test.js
 PASS   tests  packages/processor/test/api.test.js
 PASS   tests  packages/rollup/test/watch.test.js
 PASS   tests  packages/rollup/test/splitting.test.js
 PASS   tests  packages/svelte/test/svelte.test.js
(node:5494) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead
 PASS   tests  packages/webpack/test/webpack.test.js
 PASS   tests  packages/svelte/test/rollup.test.js
 PASS   tests  packages/browserify/test/factor-bundle.test.js
 PASS   tests  packages/rollup-rewriter/test/rewriter.test.js
 PASS   tests  packages/browserify/test/browserify.test.js
 FAIL   tests  packages/processor/test/composition.test.js
  ● /processor.js › composition › should fail on invalid composes syntax

    expect(received).toMatch(expected)

    Expected substring: "SyntaxError: Expected source but \"n\" found."
    Received string:    "modular-css-graph-nodes: /home/travis/build/tivac/modular-css/invalid/value.css:1:23: SyntaxError: Expected \"global\" or source but \"n\" found."

      23 |                 );
      24 |             } catch({ message }) {
    > 25 |                 expect(message).toMatch(`SyntaxError: Expected source but "n" found.`);
         |                                 ^
      26 |             }
      27 |         });
      28 | 

      at Object.toMatch (packages/processor/test/composition.test.js:25:33)

 PASS   tests  packages/processor/test/values.test.js
 PASS   tests  packages/postcss/test/postcss.test.js
 PASS   tests  packages/browserify/test/watchify.test.js
 PASS   tests  packages/processor/test/at-composes.test.js
 PASS   tests  packages/aliases/test/aliases.test.js
 PASS   tests  packages/processor/test/scoping.test.js
 PASS   tests  packages/processor/test/keyframes.test.js
 PASS   tests  packages/browserify/test/issue-58.test.js
 PASS   tests  packages/browserify/test/issue-313.test.js
 PASS   tests  packages/cli/test/cli.test.js
 PASS   tests  packages/paths/test/paths.test.js
 PASS   tests  packages/processor/test/getters.test.js
 PASS   tests  packages/processor/test/externals.test.js
 PASS   tests  packages/namer/test/namer.test.js
 PASS   tests  packages/glob/test/glob.test.js
 PASS   tests  packages/processor/test/exports.test.js
 PASS   tests  packages/processor/test/issues/issue-56.test.js
 PASS   tests  packages/processor/test/issues/issue-24.test.js
 PASS   tests  packages/processor/test/issues/issue-98.test.js
 PASS   tests  packages/processor/test/unicode.test.js
 PASS   tests  packages/processor/test/issues/issue-66.test.js
 PASS   tests  packages/processor/test/issues/issue-261.test.js

Summary of all failing tests
 FAIL  packages/processor/test/composition.test.js
  ● /processor.js › composition › should fail on invalid composes syntax

    expect(received).toMatch(expected)

    Expected substring: "SyntaxError: Expected source but \"n\" found."
    Received string:    "modular-css-graph-nodes: /home/travis/build/tivac/modular-css/invalid/value.css:1:23: SyntaxError: Expected \"global\" or source but \"n\" found."

      23 |                 );
      24 |             } catch({ message }) {
    > 25 |                 expect(message).toMatch(`SyntaxError: Expected source but "n" found.`);
         |                                 ^
      26 |             }
      27 |         });
      28 | 

      at Object.toMatch (packages/processor/test/composition.test.js:25:33)


Test Suites: 1 failed, 1 skipped, 33 passed, 34 of 35 total
Tests:       1 failed, 5 skipped, 287 passed, 293 total
Snapshots:   324 passed, 324 total
Time:        25.166s
Ran all test suites.
npm ERR! Test failed.  See above for more details.
TravisBuddy Request Identifier: a4d34cf0-d65b-11e9-82ee-13bec588d4b8
@codecov

This comment has been minimized.

Copy link

commented Sep 13, 2019

Codecov Report

Merging #669 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #669   +/-   ##
=======================================
  Coverage   99.12%   99.12%           
=======================================
  Files          45       45           
  Lines        1139     1139           
  Branches      174      174           
=======================================
  Hits         1129     1129           
  Misses         10       10

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a57cddf...1bccc36. Read the comment docs.

Copy link
Owner

left a comment

Looks great, and I'm very pleased at how straightforward the changes were 😅

You've got the checkboxes all set up correctly marking that this still needs docs, which should just be a a new example and some small updates to packages/www/src/guide/feature-composing-styles.md

parsers/shared.pegjs Outdated Show resolved Hide resolved
parsers/composes.pegjs Show resolved Hide resolved
@@ -22,10 +22,10 @@ describe("/processor.js", () => {
".a { composes: b from nowhere.css; }"
);
} catch({ message }) {
expect(message).toMatch(`SyntaxError: Expected source but "n" found.`);
expect(message).toMatch(`SyntaxError: Expected global or source but "n" found.`);

This comment has been minimized.

Copy link
@tivac

tivac Sep 13, 2019

Owner

Ugh, I wish it was easier in pegjs to customize the error messages. Nothing you need to change here, just annoyed and sharing.

@jquense

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

I was really impressed with how easy this was to do. I initially saw the generated parsers and thought it was a bit overkill, but i'm convinced now, gonna steal this strategy for other stuff!

local : "dafdfcc_other aeacf0c_single"
composable : "dafdfcc_composable",
local : "dafdfcc_composable aeacf0c_local",
removed : "dafdfcc_composable aeacf0c_local aeacf0c_removed"

This comment has been minimized.

Copy link
@jquense

jquense Sep 14, 2019

Author Contributor

I updated this to match the output of the example file above

@tivac
tivac approved these changes Sep 15, 2019
@tivac tivac merged commit 0a7996e into tivac:master Sep 15, 2019
1 of 7 checks passed
1 of 7 checks passed
triage
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
deploy/netlify Deploy preview processing.
Details
Header rules No header rules processed
Details
Pages changed 5 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
@TravisBuddy

This comment has been minimized.

Copy link

commented Sep 15, 2019

Hey @jquense,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 249a90a0-d80b-11e9-b22f-8dc32e2a8e5f
@TravisBuddy

This comment has been minimized.

Copy link

commented Sep 16, 2019

Hey @jquense,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 728d8c30-d816-11e9-94f2-eb1f1b783936
@tivac

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2019

Released in v25.0.0, thanks @jquense!

@jquense jquense deleted the jquense:from-global branch Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.