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

Proposal for configuring modules through imports #2773

Merged
merged 6 commits into from Nov 13, 2019

Conversation

@jathak
Copy link
Member

jathak commented Nov 8, 2019

This is a very rough draft of the proposal to implicitly create configurations when importing libraries that have already migrated to the module system (see #2772).

Since the semantics are just modifications of the module system spec, I just specified the sections that should be modified and stated their new contents. I'm not sure if it would be better to just directly modify that spec, but this seemed like a significant enough change that it shouldn't go through the fast-track process.

@jathak jathak force-pushed the jathak:configure-through-import branch from 10df78d to 96a907c Nov 8, 2019
@jathak jathak force-pushed the jathak:configure-through-import branch from 96a907c to f505a4c Nov 8, 2019
@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 8, 2019

This looks good so far. My main concern with this approach is the potential overhead of creating a new configuration for every @import—do you have any thoughts on how we could mitigate that?

@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 8, 2019

What if we only created the new configuration if the file being imported contains a @forward rule? Would that be sufficient?

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 8, 2019

@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 11, 2019

Okay, I added that note and also clarified that a new config created by a prefixed @forward is implicit if the original config was.

I also expanded on the design decisions section. Is there anything else you think I should bring up there?

@jathak jathak force-pushed the jathak:configure-through-import branch from d1420c3 to b5a8157 Nov 11, 2019
Copy link
Contributor

nex3 left a comment

A couple suggestions, but otherwise looks good!

One potential use case that will still not work is if a downstream user of the
library attempts to change its configuration between two imports of the same
library, that change will be ignored. However, this is an edge case that is (a)
Comment on lines 107 to 109

This comment has been minimized.

Copy link
@nex3

nex3 Nov 12, 2019

Contributor

This first sentence is kind of awkward. Consider breaking it up into two sentences instead.

probably not intended by the user, (b) relatively easy to fix by moving all
declared configuration variables before all library imports, and (c) very
difficult to support for a library using the module system without compromising
the module system's [goals][].

This comment has been minimized.

Copy link
@nex3

nex3 Nov 12, 2019

Contributor

I suggest being more specific about how it compromises which goals.

@nex3
nex3 approved these changes Nov 13, 2019
@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 13, 2019

Can this be merged? (I don't have write access to this repo)

jathak and others added 2 commits Nov 13, 2019
@nex3 nex3 merged commit 85c856e into sass:master Nov 13, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.