Better support for Yarn workspaces #1225

ringods · 2020-06-16T20:25:41Z

Closes: #1136

The code has too many places where locations are calculated into a package inside the node_modules folder. The assumption is made here that the the node_modules folder is in the current working directory (process.cwd()). But with Yarn workspaces, this is not always the case: the node_modules can be one or two levels higher in the folder structure.

Summary of changes:

Replace hardcoded path calculations with a function using require.resolve.

ringods · 2020-07-13T16:51:34Z

@sghoweri @bmuenzenmeyer loaduikits iterates over the node_modules folder for folder names containing uikit-. If there are 3 found, they are checked against the config to verify if the uikit is configured and enabled.

patternlab-node/packages/core/src/lib/loaduikits.js

Line 48 in 831c8b0

const uikits = findModules(nodeModulesPath, isUIKitModule) // [1]

patternlab-node/packages/core/src/lib/loaduikits.js

Lines 55 to 60 in 831c8b0

    
           if (!configEntry) { 
        
             logger.warning( 
        
               `Could not find uikit with name uikit-${kit.name} defined within patternlab-config.json, or it is not enabled.` 
        
             ); 
        
             return; 
        
           }

In Yarn v2, you can only load dependencies which are explicitly in your package.json file. So I'm thinking of updating the code and doing it the other way around: reading the enabled uikits and then require-ing them directly. I would error if a uikit is in the PL config, but not defined as a dependency.

If I do it this way, am I covering the same behaviour then?

bmuenzenmeyer · 2020-07-13T20:21:53Z

In Yarn v2, you can only load dependencies which are explicitly in your package.json file. So I'm thinking of updating the code and doing it the other way around: reading the enabled uikits and then require-ing them directly. I would error if a uikit is in the PL config, but not defined as a dependency.

If I do it this way, am I covering the same behaviour then?

yes all future work should read the config explictly and look for those only - the node_module scanning has lead to false-positives like uikit-polyfills

ringods · 2020-07-14T09:09:51Z

@bmuenzenmeyer I moved my resolver code from the cli package to core. After that, the require of this code in cli fails:

https://github.com/pattern-lab/patternlab-node/pull/1225/checks?check_run_id=868619630#step:4:48

I don't understand what I'm missing. Any idea?

ringods · 2020-07-26T15:27:52Z

@bmuenzenmeyer in commit 04df565, you can see the rewritten version to load the uikits. Without too much effort, the packagename can be added as a uikit config property to cover the requirements covered in #1137.

ringods · 2020-08-06T06:57:47Z

package property is added in ceccfd8, hereby copying the functionality offered in #1137

JosefBredereck

When those 3 things are resolved or clearified. I'm appy to approve this PR.

JosefBredereck · 2020-08-18T06:05:46Z

packages/core/src/lib/loaduikits.js

+    if ('package' in uikitConfig) {
+      try {
+        uikitLocation = resolvePackageFolder(uikitConfig.package);
+      } catch (ex) {
+        logger.warning(
+          `Could not find uikit with package name ${uikitConfig.package}. Did you add it to the 'dependencies' section in your 'package.json' file?`
+        );
+        return;
+      }


Please provide an example patternlab-config.json for that or change the given ones. The user shouldn't get warnings for a freshly installed pattern-lab after we merge this.

Most Important

packages/core/patternlab-config.json (used within getDefaultConfig)

packages/edition-node/patternlab-config.json

packages/edition-node-gulp/patternlab-config.json

packages/edition-twig/patternlab-config.json

Less Important or Internal

packages/core/test/util/patternlab-config.json

packages/cli/test/fixtures/patternlab-config.json

packages/development-edition-engine-handlebars/patternlab-config.json

packages/development-edition-engine-react/patternlab-config.json

packages/development-edition-engine-twig/patternlab-config.json

packages/core/test/util/patternlab-config.json is left as is intentionally because it is used in the tests for checking the warning messages.

Question back: packages/development-edition-engine-react/patternlab-config.json doesn't have a uikits block, but does point to ./node_modules/@pattern-lab/uikit-workshop files inside the paths section. With my change, this won't work in Yarn v2. I don't know if this is anything specific to React but I would need some help here on what to do.

Leave it out then. I'm not shure which engines are used anymore. Some feel depreacated / outdated anyway.

JosefBredereck · 2020-08-18T06:08:28Z

packages/core/src/lib/resolver.js

+const resolveFileInPackage = (packageName, ...pathElements) => {
+  return require.resolve(path.join(packageName, ...pathElements));
+};
+
+/**
+ * @func resolveDirInPackage
+ * Resolves a file inside a package
+ */
+const resolveDirInPackage = (packageName, ...pathElements) => {
+  return path.dirname(resolveFileInPackage(packageName, ...pathElements));
+};
+
+/**
+ * @func resolvePackageFolder
+ * Resolves the location of a package on disc
+ */
+const resolvePackageFolder = packageName => {
+  return path.dirname(resolveFileInPackage(packageName, 'package.json'));
+};


Just a question for this. The whole concept is working without process.cwd() is that correct?
When it tries to resolve the given files/directories it will walk from the resolver location through the filesystem or where would it start?

Patternlab loads files from installed NodeJS packages. It starts loading via whatever resolver a package manager could configure inside the Node runtime. In my case, I want to support Yarn v2 with PnP active eventually. In such a setup, no node_modules folder is used at all and every package is stored as a zip file. Here is an example how files inside a package are to be found:

https://yarnpkg.com/features/pnp#packages-are-stored-inside-zip-archives-how-can-i-access-their-files

If it does not interfere with other resolvers like npm or yarn v1 everything is fine.

@JosefBredereck if uikit resolution works via require.resolve, the updated code ought to work under any other setup. If it doesn't it is a bug and needs to be fixed, which I'm glad to provide a fix for.

packages/core/src/index.js

JosefBredereck · 2020-08-18T06:18:55Z

Also, you will need to update the branch from dev or the merging will be blocked.

ringods · 2020-08-18T07:59:44Z

@JosefBredereck will process your requests next week. I'm on holiday this week. ;-)

…fferent name.

ringods · 2020-08-19T05:24:26Z

@JosefBredereck can you review again? Please focus on the uikits loading for now. I will submit a new PR to update:

core/src/lib/starterkit_manager.js
core/src/lib/pattern_engines.js

so these also use the new resolver instead of calculating paths into node_modules themselves.

JosefBredereck

Good changes. Also, I like that you keep the old way in place and notify the user to update the config.

ringods requested a review from raphaelokon as a code owner June 16, 2020 20:25

ringods self-assigned this Jun 16, 2020

ringods marked this pull request as draft June 16, 2020 20:25

ringods force-pushed the feature/yarn-workspaces branch from 988136b to a22809e Compare July 13, 2020 16:18

ringods requested a review from sghoweri July 13, 2020 16:21

ringods mentioned this pull request Jul 13, 2020

Allow uikit plugins to be used in multiple uikit definitions in patternlab-config.json. #1137

Closed

ringods force-pushed the feature/yarn-workspaces branch 2 times, most recently from e0cdb11 to 201014d Compare July 26, 2020 13:10

ringods mentioned this pull request Jul 26, 2020

Github Actions over Travis-CI? #1185

Closed

JosefBredereck added the ready for review label Aug 17, 2020

JosefBredereck requested changes Aug 18, 2020

View reviewed changes

JosefBredereck removed request for raphaelokon and sghoweri August 18, 2020 06:17

JosefBredereck marked this pull request as ready for review August 18, 2020 06:17

ringods force-pushed the feature/yarn-workspaces branch from 7ed5244 to e694c37 Compare August 19, 2020 05:10

ringods added 6 commits August 19, 2020 07:19

Use NodeJS require module to resolve packages and files/folders in it

31bc3f4

Moving the resolver functions to the core package

78a9cb1

Rewrote loading the UI kits starting from the config

b2df7ee

Adding package property to allow a uikit to be used twice with a di…

04a835e

…fferent name.

Remove obsolete comment

4bc0881

Prevent uikit warnings on our own packages

4b18d43

ringods force-pushed the feature/yarn-workspaces branch from e694c37 to 4b18d43 Compare August 19, 2020 05:19

ringods requested a review from JosefBredereck August 19, 2020 05:21

ringods mentioned this pull request Aug 19, 2020

Add Preliminary Support For Javascript-based Configs #1000

Closed

4 tasks

JosefBredereck approved these changes Aug 20, 2020

View reviewed changes

JosefBredereck added Approved by Member and removed ready for review labels Aug 20, 2020

ringods merged commit 0cbdac2 into dev Aug 23, 2020

ringods deleted the feature/yarn-workspaces branch August 23, 2020 15:19

sghoweri mentioned this pull request Aug 23, 2020

Revert "Better support for Yarn workspaces" #1245

Merged

sghoweri restored the feature/yarn-workspaces branch August 23, 2020 15:50

ringods deleted the feature/yarn-workspaces branch August 26, 2020 13:14

Better support for Yarn workspaces #1225

Better support for Yarn workspaces #1225

Uh oh!

Conversation

ringods commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ringods commented Jul 13, 2020

Uh oh!

bmuenzenmeyer commented Jul 13, 2020

Uh oh!

ringods commented Jul 14, 2020

Uh oh!

ringods commented Jul 26, 2020

Uh oh!

ringods commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JosefBredereck left a comment

Choose a reason for hiding this comment

Uh oh!

JosefBredereck Aug 18, 2020 • edited by ringods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ringods Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

JosefBredereck Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

JosefBredereck Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

ringods Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

JosefBredereck Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

ringods Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JosefBredereck commented Aug 18, 2020

Uh oh!

ringods commented Aug 18, 2020

Uh oh!

ringods commented Aug 19, 2020

Uh oh!

JosefBredereck left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ringods commented Jun 16, 2020 •

edited

Loading

ringods commented Aug 6, 2020 •

edited

Loading

JosefBredereck Aug 18, 2020 •

edited by ringods

Loading