Build: Use node script to symlink local npm dependencies by gziolo · Pull Request #79 · WordPress/packages

gziolo · 2018-02-09T20:30:28Z

This PR addresses recommendation from @ntwb shared in #77:

Regarding the symlinks, maybe we could use Lerna's symlink function or replicate similar functionality here in /packages so that we can include Windows compatible symlinks

https://github.com/lerna/lerna/blob/master/src/FileSystemUtilities.js#L103-L110
Alternatively, https://www.npmjs.com/package/symlink-or-copy could be used

There's quite a few symlink packages available https://www.npmjs.com/browse/keyword/symlink, I'm surprised Lerna doesn't actually use one of these packages.

I haven't tried Lerna symlink as it doesn't look promising in case of the root folder. I would expect that their author would mention it in the discussion we had before.

However symlink-or-copy turned out to work pretty well with some tweaks to ensure it doesn't fail because of its limitations. This should be platform independent. This time we need to test on Windows to confirm :)

Testing

Run npm i on a fresh git clone and npm test. Run npm i a few more times and make sure it still works.

It is also a good idea to run npm run publish:dev and npm run publish:prod without publishing packages, just to make sure if everything works as expected.

aduth · 2018-02-09T21:00:31Z

+		[ 'packages/scripts', 'node_modules/@wordpress/scripts' ],
+		[ 'node_modules/@wordpress/scripts/bin/wp-scripts.js', 'node_modules/.bin/wp-scripts' ],
+	];
+	symlinksToCreate.forEach( ( [ srcPath, destPath ] ) => {


I guess there's no async version of this module? Otherwise seems like it'd be simple / advisable to write as a Promise.all

From README of this npm package:

There intentionally isn't an asynchronoukks version. It's not clear that we need or want one. Before sending a patch to add an async version, please share your use case on the issue tracker.

They provide only sync version.

ntwb · 2018-02-09T22:15:09Z

 		"prebuild": "check-node-version --package",
 		"build": "node ./scripts/build.js",
-		"postinstall": "lerna bootstrap --hoist && ./scripts/create-symlinks.sh && npm run build",
+		"postinstall": "lerna bootstrap --hoist && node ./scripts/create-symlinks.js && npm run build",


Let's add a new script similar to the build script:

"build": "node ./scripts/build.js", "create-symlinks": "node ./scripts/create-symlinks.js", "postinstall": "lerna bootstrap --hoist && npm run create-symlinks && npm run build",

I will 👍

I also added npm-run-all to simplify run scripts with 4f363b1.

ntwb · 2018-02-09T23:43:09Z

Testing this using Hyper terminal on Windows 10:

(Skip to the end of this comment to get to the good bits avoiding having to avoid reading the debug messages)

Before this PR:

C:\Users\Stephen\dev\@wordpress\packages>npm i

> undefined postinstall C:\Users\Stephen\dev\@wordpress\packages
> lerna bootstrap --hoist && ./scripts/create-symlinks.sh && npm run build

lerna info version 2.8.0
lerna info versioning independent
lerna info Bootstrapping 10 packages
lerna info lifecycle preinstall
lerna info Installing external dependencies
lerna info hoist Installing hoisted dependencies into root
lerna info hoist Pruning hoisted dependencies
lerna info hoist Finished pruning hoisted dependencies
lerna info hoist Finished installing in root
lerna info Symlinking packages and binaries
lerna info lifecycle postinstall
lerna info lifecycle prepublish
lerna info lifecycle prepare
lerna success Bootstrapped 10 packages
'.' is not recognized as an internal or external command,
operable program or batch file.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! undefined postinstall: `lerna bootstrap --hoist && ./scripts/create-symlinks.sh && npm run build`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the undefined postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Stephen\AppData\Roaming\npm-cache\_logs\2018-02-09T22_56_36_088Z-debug.log

This PR: Fails with the '.' is not recognized as an internal or external command, error

C:\Users\Stephen\dev\@wordpress\packages>npm i

> undefined postinstall C:\Users\Stephen\dev\@wordpress\packages
> lerna bootstrap --hoist && ./scripts/create-symlinks.sh && npm run build

lerna info version 2.8.0
lerna info versioning independent
lerna info Bootstrapping 10 packages
lerna info lifecycle preinstall
lerna info Installing external dependencies
lerna info hoist Installing hoisted dependencies into root
lerna info hoist Pruning hoisted dependencies
lerna info hoist Finished pruning hoisted dependencies
lerna info hoist Finished installing in root
lerna info Symlinking packages and binaries
lerna info lifecycle postinstall
lerna info lifecycle prepublish
lerna info lifecycle prepare
lerna success Bootstrapped 10 packages
'.' is not recognized as an internal or external command,
operable program or batch file.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! undefined postinstall: `lerna bootstrap --hoist && ./scripts/create-symlinks.sh && npm run build`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the undefined postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Stephen\AppData\Roaming\npm-cache\_logs\2018-02-09T22_56_36_088Z-debug.log

This PR with the change noted in the PR review here #79 (comment)

> node ./scripts/create-symlinks.js


Creating symlinks…
fs.js:1014
  return binding.symlink(preprocessSymlinkDestination(target, type, path),
                 ^

Error: EPERM: operation not permitted, symlink 'C:\Users\Stephen\dev\@wordpress\packages\packages\browserslist-config' -> 'C:\Users\Stephen\dev\@wordpress\packages\node_modules\@wordpress\browserslist-config'
    at Object.fs.symlinkSync (fs.js:1014:18)
    at symlinkWindows (C:\Users\Stephen\dev\@wordpress\packages\node_modules\symlink-or-copy\index.js:120:16)
    at symlinkOrCopySync (C:\Users\Stephen\dev\@wordpress\packages\node_modules\symlink-or-copy\index.js:63:5)
    at symlinksToCreate.forEach (C:\Users\Stephen\dev\@wordpress\packages\scripts\create-symlinks.js:20:4)
    at Array.forEach (<anonymous>)
    at mkdirp (C:\Users\Stephen\dev\@wordpress\packages\scripts\create-symlinks.js:18:19)
    at C:\Users\Stephen\dev\@wordpress\packages\node_modules\mkdirp\index.js:48:26
    at FSReqWrap.oncomplete (fs.js:153:5)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @ create-symlinks: `node ./scripts/create-symlinks.js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the @ create-symlinks script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Stephen\AppData\Roaming\npm-cache\_logs\2018-02-09T23_13_57_118Z-debug.log

Turns out there is an issue upstream broccolijs/node-symlink-or-copy#35 that prevents Windows directories being symlinked broccolijs/node-symlink-or-copy#35

There is a pull request here that fixes the issue broccolijs/node-symlink-or-copy#36

Changing the installed version of the module to that PR:
• "symlink-or-copy": "github:broccolijs/node-symlink-or-copy#pull/36/head"

Results in:

$ npm run create-symlinks

> @ create-symlinks C:\Users\Stephen\dev\@wordpress\packages
> node ./scripts/create-symlinks.js


Creating symlinks…
DONE

Unfortunately, there is an issue with npm 5.x and installing the pull request branch of symlink-or-copy and can only be used with npm 3.x or 4.x /shrug

Once the upstream issue is merged we can update this PR and everything will work as we'd hoped, which is shown in this screenshot the correct modules symlinked in /node_modules/@wordpress

gziolo · 2018-02-12T12:14:18Z

@ntwb let's wait a few days until node-symlink-or-copy has this fixed and new version is published. If they don't fix by next week, we might use a fork for the time being.

ntwb · 2018-02-13T03:16:07Z

Agreed, waiting is fine, the issue is only related to Windows and I don't think there are many, if any Windows users currently using this repo, though maintaining compatibility for Windows users should be a continuing goal for this repo

stefanpenner · 2018-02-15T04:53:37Z

@gziolo / @ntwb I just released node-symlink-or-copy with the fix as v1.2.0 🎉

gziolo · 2018-02-15T10:03:07Z

@stefanpenner thanks for update and ping. I updated this PR. It is ready for another round of testing.

Ready for another review with updated node-symlink-or-copy.

gziolo · 2018-02-15T10:04:52Z

 		"test:watch": "npm run test -- --watch",
-		"publish:dev": "npm run build-clean && npm run build && lerna publish --npm-tag next",
-		"publish:prod": "npm run build-clean && npm run build && lerna publish"
+		"publish:dev": "npm-run-all build-clean build && lerna publish --npm-tag next",


I know it's unrelated, but I felt tempted to refactor as I added npm-run-all 😃

If build-clean is a preparation step for build, why not include in prebuild ?

I think it makes sense to do it. I will give it a spin.

stefanpenner · 2018-02-15T20:19:55Z

This shouldn't affect your PR, but I have a follow up PR to node-symlink-or-copy to optimize the cases where directories are not linkable but files are. broccolijs/node-symlink-or-copy#37

I am amazed by how windows handles this...

gziolo · 2018-02-19T10:28:28Z

@stefanpenner, thanks for letting us know. Thanks for making windows platform more accessible for node devs :)

ntwb

LGTM 👍

gziolo added enhancement Build Tooling labels Feb 9, 2018

gziolo self-assigned this Feb 9, 2018

gziolo requested a review from ntwb February 9, 2018 20:30

aduth reviewed Feb 9, 2018

View reviewed changes

ntwb previously requested changes Feb 9, 2018

View reviewed changes

ntwb added the [Status] Blocked label Feb 9, 2018

ntwb mentioned this pull request Feb 9, 2018

fix #35 broccolijs/node-symlink-or-copy#36

Merged

gziolo added 2 commits February 15, 2018 10:47

Build: Use node script to symlink local npm dependencies

f5fbc13

Chore: Update symlink-or-copy to v1.2.0

4a9ad37

gziolo force-pushed the update/npm-deps-file branch from cbeba0d to 4a9ad37 Compare February 15, 2018 09:48

gziolo commented Feb 15, 2018

View reviewed changes

gziolo removed the [Status] Blocked label Feb 15, 2018

Chore: Added npm-run-all to simplify npm scripts

8862f36

gziolo force-pushed the update/npm-deps-file branch from 4f363b1 to 8862f36 Compare February 15, 2018 13:34

Merge master

8e5b10f

ntwb approved these changes Feb 19, 2018

View reviewed changes

gziolo merged commit 05e178c into master Feb 20, 2018

gziolo deleted the update/npm-deps-file branch February 20, 2018 06:37

aduth removed the enhancement label May 7, 2018

Uh oh!

Conversation

gziolo commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntwb commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntwb commented Feb 13, 2018

Uh oh!

stefanpenner commented Feb 15, 2018

Uh oh!

gziolo commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented Feb 15, 2018

Uh oh!

gziolo commented Feb 19, 2018

Uh oh!

ntwb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gziolo commented Feb 9, 2018 •

edited

Loading

ntwb commented Feb 9, 2018 •

edited

Loading

gziolo commented Feb 12, 2018 •

edited

Loading