Update loom to v1 #4544
Merged
Update loom to v1 #4544
Conversation
This file contains 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
|
👋 It looks like you're updating JavaScript packages that are known You can deduplicate them with the A duplicate React version may cause an invalid hook call warning. React context providers usually use module-scoped globals as their |
size-limit report
|
dahukish
reviewed
Oct 26, 2021
| // them to work here either | ||
| configuration.jestModuleMapper?.hook((moduleMapper) => { | ||
| // them to work in jest tests either | ||
| configuration.jestModuleNameMapper?.hook((moduleMapper) => { |
There was a problem hiding this comment.
should everything else follow suit moduleMapper => moduleNameMapper
There was a problem hiding this comment.
the naming of the argument isn't suuuper important as it's highly localised. I think leaving as moduleMapper gets the point across.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WHY are these changes introduced?
Updating loom to v1
WHAT is this pull request doing?
require()andexportand thus mixed cjs and esm and relied on transpiling to sort it out which i think is silly.How to 🎩
mv build build-ogyarnto update dependencies, run ayarn cleanto delete the build folderyarn buildto generate a new builddiff -ru -x '*.tsbuildinfo' build-og buildto compare the old and new build output.What about loom-plugin-build-extended?
An excellent question, how good of you to ask.
loom-plugin-build-extendedincludes support for loading scss files using node-sass. Its rollup plugin for handling styles is based upon the styles rollup plugin in polaris-react. However I am choosing not to remove this repo's styles plugin in favour of loom-plugin-build-extended.This is because we're intending to replace
node-sasswithdart-sassinloom-plugin-build-extendedin the near future. For most projects, that's completely fine - they fix any migration from node-sass to dart-sass and they're good. However polaris-react ships a sass public api that needs to be understandable in apps that use node-sass (as sewing-kit still ships with node-sass) and within the polaris-react codebase.Migrating this codebase to dart-sass is possible, however it raises deprecation warnings in the publicly used functions that can only be fixed by migrating the codebase to dart-sass and those changes will throw errors when used by apps that leverage node-sass.
In order to migrate this codebase to dart-sass we need to either: remove the public sass API, or make sure that all consuming apps have migrated to use dart-sass using sewing-kit's dartSassLoader experiment (this is huge effort and a pain, just remove the public sass api please).
In order to not be in a place where this codebase is constantly raising deprecation warnings that can't be fixed without breaking consumers it's better to leave polaris-react using node-sass for the time being. Which means sticking with it having its own style loader.