refactor: avoid cross-package relative Sass imports to support Yarn PnP #24536
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devversion
added
merge: preserve commits
devversion
requested review from
a team,
andrewseguin,
crisbeto,
jelbourn and
mmalerba
as code owners
1 task
crisbeto
reviewed
Mar 14, 2022
| font-size, line-height, font-weight, letter-spacing, font-family, font-shorthand; | ||
|
|
||
| // Private/Internal | ||
| @forward './core/density/private/all-density' as private-* show private-all-component-densities; |
Member
There was a problem hiding this comment.
Instead of exposing these through the main entry point, should we have a separate entry point instead?
Member
Author
There was a problem hiding this comment.
Yes, I could do that, but it has two consequences:
- Need for maintaining/wiring up another file in the NPM package (e.g. in the
exports) - Increase in inconsistency for cross-package imports (where currently there is just a single way)
Also people might still import from it. I believe there is not much benefit, or do you have other thoughts?
devversion
force-pushed
the
yarn-pnp
branch
2 times, most recently
from
4b1b8a3 to
cbde4a2
Compare
andrewseguin
approved these changes
Mar 16, 2022
Contributor
|
Needs rebase - this needs some special care getting into Google. Let me know the next time you have this rebased so I can focus on it |
Contributor
|
Sorry @devversion - needs another little rebase. I think I've got the internal stuff almost working so it shouldn't be much longer |
Our new Yarn PnP integration test unveiled some issues with the tar archive creation that break within Yarn berry. The problem is that Yarn expects a non-collapsing first archive member component. e.g. currently it is: `./package.json`, but Yarn normalizes the path and therefore omits files like the `package.json` completely. The archive member (to match with `yarn pack`) should instead use an actual first component, like `package/package.json`.
Workaround until https://docs.google.com/document/d/1yVRAJptClM1YNH2XqdpPOef9rmLMVeo_qTJLM5j1gDo/edit is addressed/resolved. We need to tell Sass how `@angular/<cdk,material,etc>` imports can be resolved so that we can start using them, similar to how NPM-consumers would. This is a key part of making our NPM packages work with Yarn berry, or more generally making our packages not rely on unspecified behavior (our package exports should not allow for deep imports, but currently we rely on those!)
Yarn PnP requires explicit listing of dependencies used. Since our Sass files/compilations do not use `material-components-web` but rather use the individual `@material/<..>` packages, we need to switch away from the kitchen-sink package. As part of this, we have two options long-term: * Add all individual deps as peer dependencies * Add all individual deps as _direct_ dependencies. The latter seems more convenient for users and we also cannot make any guarantees that a more recent "build" of the `@material/<..>` packages is still compatible. If users would intend to force a more recent MDC version, they could still use a Yarn resolution, or we could also bump our `dependencies` using a `caret` or `tilde` (once we are relying on a rather stable version of MDC -- that is a potential option then)
Exposes the `datepicker-date-range-colors` mixin which seems to be not accessible in the primary Sass entry-point, but being mentioned in the docs.
… mixin Exposes the `dialog-legacy-padding` mixin which seems to be not accessible in the primary Sass entry-point, but being mentioned in the docs.
…tive Sass imports
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #24128
See individual commits. Note that this is not necessarily only being about Yarn 2.x+, but can also
be considered a general cleanup, enforcing consistent ways of importing code between packages.
Also, in the future, deep cross-NPM package relative imports might break regardless (if not explicitly specified
in the
package.json#exports- adding all files is counterintuitive). We are currently relying on unspecified behavior that can change at anytime when e.g.
webpacksass-loader changes, or when NPM packages are laid out differently etc.As part of this change, I cleaned-up most missing Sass dependencies. One downside of this change is
that most Sass files now rely on the larger bulk
_index.scssfiles (and its transtive libs) (see note #2 in details)Note #1: For Yarn PnP support/or our NPM packages to be future-proof/valid in respect to package exports, it would have been sufficient to only update the theming Sass files. I've decided to also apply the changes to our component Sass files (sass binaries) since that will make the importing consistent/easier to remember (and the lint rule also)
Note #2: This will increase the action input for some Sass binaries, but that seems acceptable given that all unit tests
rely on a prebuilt-theme anyway etc. This will also match with what NPM users are seeing. Certainly a little
unfortunate to give-up on a little fine-grained targets here but we haven't been very consistent with that anyay.. (and it's a trade-off)