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.

Sign up for GitHub

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

Jump to bottom

Note configurations are shared across all files in a module by instance given a uniquely-identifying opaque ID #3357

Merged
merged 3 commits into from Aug 17, 2022
Merged

Note configurations are shared across all files in a module by instance given a uniquely-identifying opaque ID #3357

merged 3 commits into from Aug 17, 2022

Conversation

Goodwine
Copy link
Member

@Goodwine Goodwine commented Jul 13, 2022 •
edited

@Goodwine Goodwine requested a review from nex3 July 13, 2022 22:58
@Goodwine Goodwine mentioned this pull request Jul 13, 2022
Closed
@nex3
Copy link
Contributor

nex3 commented Jul 14, 2022

Since the module system proposal has already been implemented, this should edit the spec itself (in spec/module-system.md).

This was referenced Jul 14, 2022
Merged
Merged
@Goodwine Goodwine force-pushed the proposal.module-system.draft-10.1 branch from 4d596b7 to 4803fc5 Compare July 14, 2022 02:07
@Goodwine
Copy link
Member Author

Gotcha, I actually wasn't sure following the contributing process, I have updated the pr :)

nex3
nex3 reviewed Jul 14, 2022
View reviewed changes
spec/modules.md Outdated Show resolved Hide resolved
spec/modules.md Outdated Show resolved Hide resolved
@Goodwine Goodwine requested a review from nex3 July 15, 2022 19:44
nex3
nex3 requested changes Jul 15, 2022
View reviewed changes
spec/spec.md Outdated Show resolved Hide resolved
@nex3
Copy link
Contributor

nex3 commented Jul 15, 2022 •
edited

Another thing we should bear in mind here is that different @forward rules can actually change configured values. For example, the following should still throw an error:

<===> input.scss
@use "downstream" with ($var1: from input);

<===> _downstream.scss
@forward "upstream" with ($var2: from downstream 1);
@forward "upstream" with ($var2: from downstream 2);

<===> _upstream.scss
$var1: from upstream !default;
$var2: from upstream !default;

...so simply checking that two configurations come from the same original configuration isn't sufficient.

@Goodwine
Copy link
Member Author

It does fail as expected because while @use .. with seems to be OK with it (although I'm not 100% sure), the @forward .. with does not seems to be OK with it, guessing because they are not the same nodespan

sass/sass-spec@908b6a0

@Goodwine Goodwine force-pushed the proposal.module-system.draft-10.1 branch from ef46c38 to 8168a34 Compare July 20, 2022 02:10
@Goodwine
Copy link
Member Author

PTAL

@Goodwine Goodwine requested a review from nex3 July 20, 2022 02:10
nex3
nex3 reviewed Jul 22, 2022
View reviewed changes
spec/modules.md Outdated Show resolved Hide resolved
spec/modules.md Outdated Show resolved Hide resolved
@Goodwine Goodwine force-pushed the proposal.module-system.draft-10.1 branch 2 times, most recently from e130f7e to 7f10963 Compare July 26, 2022 21:49
@Goodwine
'@use .. with' Configs get unique IDs propagated down with @forwards
6c12f71 
When loading a module multiple times, like when several sources end up
using the modules, it used to be an error but now it's allowed as long
as the configuration is the same according to its ID
@Goodwine Goodwine force-pushed the proposal.module-system.draft-10.1 branch from 7f10963 to 6c12f71 Compare July 26, 2022 21:50
@Goodwine Goodwine requested a review from nex3 August 3, 2022 18:44
nex3
nex3 reviewed Aug 4, 2022
View reviewed changes
spec/at-rules/forward.md Outdated Show resolved Hide resolved
spec/modules.md Outdated Show resolved Hide resolved
@Goodwine Goodwine requested a review from nex3 August 8, 2022 21:51
nex3
nex3 approved these changes Aug 9, 2022
View reviewed changes
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more small change, then looks good! Thanks for bearing with me through so many iterations.

spec/modules.md Outdated Show resolved Hide resolved
@Goodwine
Copy link
Member Author

Thanks for the reviews!

@Goodwine Goodwine merged commit 898d716 into main Aug 17, 2022
@Goodwine Goodwine deleted the proposal.module-system.draft-10.1 branch August 17, 2022 19:34
mirisuzanne added a commit that referenced this pull request Aug 26, 2022
@mirisuzanne
Merge branch 'main' into color-4-formats
0223a56 
* main: (41 commits)
  [Function Units] Add a proposal (#3375)
  Mark accepted: Random with Units Proposal (#3377)
  Note configurations are shared across all files in a module by instance given a uniquely-identifying opaque ID (#3357)
  Clarify that compile and compileString accept optional options (#3373)
  Proposal: Define random() behavior for numbers with units (#3361)
  [Media Logic] Apply changes to the spec (#3365)
  [Reconfigurable Modules] Apply the proposal to the spec (#3364)
  [Media Logic] Mark as accepted (#3360)
  [Bogus Combinators] Update the specification (#3362)
  Update documentation for charset option (#3358)
  Define "useless selectors" in Phase 1
  Support complex selectors composed of only a single combinator
  Throw an error for an @extend rule with a bogus extender or target
  Don't throw an error for bogus style rules with no children
  Only omit style rules if _all_ of their complex selectors are bogus
  Allow bogus selectors in `selector.append()`
  Support single leading combinators in Phase 1
  Clarify the definition of bogus selectors in Phase 2
  Update the timeline for the deprecation and removal of `@import` (#3354)
  Mark the bogus combinators proposal as accepted
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nex3 nex3 nex3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Goodwine @nex3