PNP rule #1

danjuv · 2019-11-21T05:18:13Z

This PR attempts to add PnP support to the yarn_install rule in bazel. PnP is designed to replace the node_modules folder with a lookup table pointing directly to the yarn cache..

The core idea behind the design is to emulate the way PnP resolving works by providing the integrity checksum as input to bazel instead of individual files. Which means we save time normally spent symlinking node_modules into the sandbox/repository directories since we only need to pass around a solitary .pnp.js file (the file containing the lookup table).

The main drawback of this design is the pnp file references the yarn cache, which isn't sandboxed by bazel (and we don't want it to, since that would be more time consuming than fine-grained node_modules). While bazel cannot guarantee the hermeticity of the yarn cache, determinism is a core feature of yarn, so the rules as a whole will still be deterministic.

uri-canva

@joscha can you review the typescript code to make sure it's idiomatic?
@toby does this make sense to you?

uri-canva · 2019-11-21T05:31:25Z

internal/node/node.bzl

@@ -239,6 +239,7 @@ def _nodejs_binary_impl(ctx):
        "TEMPLATED_repository_args": _to_manifest_path(ctx, ctx.file._repository_args),
        "TEMPLATED_script_path": _to_execroot_path(ctx, ctx.file.entry_point),
        "TEMPLATED_vendored_node": "" if is_builtin else strip_external(ctx.file._node.path),
+        "TEMPLATED_pnp_file": _to_manifest_path(ctx, ctx.file.pnp_file),


This doesn't need to be its own substitution: if we use this we don't use TEMPLATED_bazel_require_script, so you can replace that with this when pnp_file is set.

I think we'd need to if we're going to keep this backwards compatible though

I think I'd prefer if it has its own substitution, it makes it easier in the launcher script to verify if we are in PnP mode or not. WDYT?

uri-canva · 2019-11-21T05:34:41Z

internal/node/node_launcher.sh

@@ -207,7 +206,7 @@ if [ "${EXPECTED_EXIT_CODE}" -eq "0" ]; then
  # handled by the node process.
  # If we had merely forked a child process here, we'd be responsible
  # for forwarding those OS interactions.
-  exec "${node}" "${NODE_OPTIONS[@]}" "${MAIN}" "${ARGS[@]}"
+  exec "${node}" "${ARGS[@]}" "${NODE_OPTIONS[@]}" "$(pwd)/TEMPLATED_script_path"


Why the change from MAIN to $(pwd)/TEMPLATED_script_path? Does it not work with the relative path? If so why not change the value of MAIN?

because MAIN is a node script that patches the resolver to fix the resolve order for node_modules for some cases (like Angular). Since we don't have node modules anymore I replaced that with just invoking the entry script directly.

We can change the value of MAIN though, that will also work

uri-canva · 2019-11-21T05:42:54Z

internal/npm_install/generate_pnp_build_files.ts

+
+filegroup(
+name = "${pkg}",
+srcs = []


Add a comment here explaining that the srcs are intentionally empty, otherwise it would look like a mistake.

uri-canva · 2019-11-21T05:44:16Z

internal/npm_install/generate_pnp_build_files.ts

+  const contents = `package(default_visibility = ["//visibility:public"])
+exports_files([
+  ".pnp.js",
+  "package.json",


4 space indent in BUILD.bazel files.

uri-canva · 2019-11-21T05:44:30Z

internal/npm_install/generate_pnp_build_files.ts

+load("@build_bazel_rules_nodejs//internal/npm_install:node_module_library.bzl", "node_module_library")
+
+filegroup(
+name = "${pkg}",


4 space indent in BUILD.bazel files.

uri-canva · 2019-11-21T05:46:03Z

internal/npm_install/generate_pnp_build_files.ts

+): Promise<Set<string>> {
+  const flattenedDeps: Set<string> = new Set();
+  deps.forEach((key: string) => {
+    // loop over transitive dependencies here


Remove this comment, it's not relevant anymore.

uri-canva · 2019-11-21T05:46:55Z

internal/npm_install/generate_pnp_build_files.ts

+
+  flattenedDeps.forEach(createBuildFile);
+
+  processDependencies(devDependencies, "devDependencies");


We should create build files for dev dependencies too. Are there also test dependencies or other dependencies types we should handle?

uri-canva · 2019-11-21T05:48:34Z

internal/npm_install/npm_install.bzl


-def _create_build_files(repository_ctx, rule_type, node, lock_file):
+]


This is empty? Is this intentional?

uri-canva · 2019-11-21T05:50:08Z

internal/npm_install/process_pnp_file.ts

+
+  const pnpPath = path.join(workspacePath, ".pnp.js")
+  const pnpFile = fs.readFileSync(pnpPath, "utf8");
+  const patchedPnpFile = pnpFile.replace("issuerModule ? issuerModule.filename : `${process.cwd()}/`;", `"${process.cwd()}"`);


Add a comment explaining the intended replacement here. Otherwise it's very confusing.

uri-canva

Add an explanation of the design of the change in the PR description. Copy that in the commit message too, so it will show up in the patch when you create it with git format-patch.

uri-canva · 2019-11-21T05:54:39Z

internal/npm_install/npm_install.bzl

-        "1" if error_on_build_files else "0",
-        repository_ctx.path(lock_file),
-        ",".join(repository_ctx.attr.included_files),
-    ])


This should be changed to execute the new generator when using pnp, but still running the old one when not.

uri-canva · 2019-11-21T05:55:52Z

internal/npm_install/npm_install.bzl

+        repository_ctx.path(repository_ctx.attr.package_json.package + "/package.json"),
+    ])
+    if result.return_code:
+        fail("deleting postinstall scripts failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (result.stdout, result.stderr))


Why deleting the install scripts? Did this come from the other patch?

yes, this code came from the yarn_install patch in the canva repo

uri-canva · 2019-11-21T05:59:58Z

internal/npm_install/npm_install.bzl


    args = [
        repository_ctx.path(yarn),
        "--cwd",
        root,
        "--network-timeout",
        str(repository_ctx.attr.network_timeout * 1000),  # in ms
+        "--modules-folder",
+        repository_ctx.path("node_modules"),


What is this change for? Did it come from another patch?

danjuv · 2019-11-22T05:25:49Z

@uri-canva changes in

uri-canva · 2019-11-22T06:59:00Z

internal/node/node.bzl

    substitutions = {
        "TEMPLATED_args": " ".join([
            expand_location_into_runfiles(ctx, a)
            for a in templated_args
        ]),
+


Spurious change, revert: the smaller the change the easier it will be to apply cleanly when we update the baseline.

uri-canva · 2019-11-22T07:27:09Z

internal/node/node.bzl

@@ -258,15 +259,15 @@ def _nodejs_binary_impl(ctx):
    # entry point is only needed in runfiles if it is a .js file
    if ctx.file.entry_point.extension == "js":
        runfiles = depset([ctx.file.entry_point], transitive = [runfiles])
-
+    run_files = [ctx.outputs.loader]


In this context there's now a variable called runfiles and one called run_files. That is too confusing.

uri-canva · 2019-11-22T07:28:21Z

internal/node/node.bzl

-                files = node_tool_files + [
-                            ctx.outputs.loader,
-                        ] + ctx.files._source_map_support_files +
+                files = node_tool_files + run_files + ctx.files._source_map_support_files +


Why are node_tool_files, node_modules and sources added to both files and transitive_files? Does pnp_file also need added to both? Or do neither?

not sure, i tried to change as little as possible, since even if node_modules lists are passed around, they're going to be empty

uri-canva · 2019-11-22T07:31:33Z

internal/node/node_launcher.sh

 ALL_ARGS=(TEMPLATED_args $NODE_REPOSITORY_ARGS "$@")
 for ARG in "${ALL_ARGS[@]}"; do
  case "$ARG" in
    --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;;
    --nobazel_patch_module_resolver)
      MAIN="TEMPLATED_script_path"
-      NODE_OPTIONS+=( "--require" "$bazel_require_script" )


Restore this to stay backwards compatible.

uri-canva · 2019-11-22T07:32:28Z

internal/node/node_launcher.sh

+  MAIN="TEMPLATED_script_path"
+  NODE_OPTIONS=("--require" "${pnp_file}")
+fi
+
 ALL_ARGS=(TEMPLATED_args $NODE_REPOSITORY_ARGS "$@")
 for ARG in "${ALL_ARGS[@]}"; do
  case "$ARG" in
    --bazel_node_modules_manifest=*) MODULES_MANIFEST="${ARG#--bazel_node_modules_manifest=}" ;;
    --nobazel_patch_module_resolver)


I see this is conditional on passing --nobazel_patch_module_resolver from https://github.com/bazelbuild/rules_nodejs/blob/5b4e4a0fa403af0aa201ee94e00e9807daee6dbd/internal/providers/node_runtime_deps_info.bzl#L62 can our code do the same, rather than looking for pnp_file?

uri-canva · 2019-11-22T07:36:55Z

internal/npm_install/npm_install.bzl

+    "JAVA_HOME",
+    "LDFLAGS",
+    "PATH",
+]


Rather than including the changes in the patches we already use, create a new branch with them, and rebase this branch on that branch, so the PR only shows changes in the current patch, not all patches together.

I had a go at this, it seems to introduce too many merge conflicts to the point that I don't think it's too worthwhile

This will be necessary once we apply the patches in the repo anyway: we don't want to specify a single patch that contains all the changes we made to upstream, we want a separate patch for each logical change, so we know when we can stop patching them in because upstream have merged our PRs, or because we don't need them anymore.

good point, i didnt think about it that way, i'll split this out

uri-canva · 2019-11-22T08:23:06Z

internal/npm_install/npm_install.bzl

-
-    repository_ctx.template(
+    
+    repository_ctx.template( 


Spurious change, revert (it might require editing the file in an editor that doesn't strip trailing whitespace from lines).

uri-canva · 2019-11-22T08:25:51Z

internal/npm_install/npm_install.bzl

@@ -267,6 +309,7 @@ cd "{root}" && "{npm}" {npm_args}
    )
    if result.return_code:
        fail("pre_process_package_json.js failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (result.stdout, result.stderr))
+    is_pnp = result.stdout.strip()


It looks like there is a json parsing library for starlark: bazelbuild/bazel#3732 (comment)

Please use that instead of this, this is too hacky.

joscha · 2019-11-25T04:45:22Z

internal/npm_install/generate_pnp_build_files.ts

+  writeFileSync(`BUILD.bazel`, contents);
+}
+
+main();


inject process.argv here:

Suggested change

main();

main(process.argv[3]);

so that the file can be required without side-effects or potentical errors.

or ideally call main from a CLI entry file.

joscha · 2019-11-25T04:49:20Z

internal/npm_install/generate_pnp_build_files.ts

+
+const workspacePath = process.argv.slice(2)[0];
+
+function mkdirp(p: string) {


Node 10.12 has fs.mkdir(..., { recursive: true }) - do we need to support older versions?

joscha · 2019-11-25T04:51:07Z

internal/npm_install/generate_pnp_build_files.ts

+  writeFileSync(`${pkg}/BUILD.bazel`, contents);
+}
+
+async function main() {


Suggested change

async function main() {

async function main(workspacePath: string) {

joscha · 2019-11-25T04:51:47Z

internal/npm_install/generate_pnp_build_files.ts

+  }
+}
+
+function writeFileSync(p: string, content: string) {


OOI: do all these need to be synchronous?

Also nit: (p: string, content: string) is a much more narrow signature than the original writeFileSync on fs. You could either inline the mkdirp calls (there are only two invocations anyway) or make your own writeFileSync a typeof fs.writeFileSync to align the signatures. I'd probably do the former.

Not necessarily, but I didn't particularly want to deal with callbacks, but I see now fs has a promises property in node@^10, so it might be better to just use that

joscha · 2019-11-25T04:52:31Z

internal/npm_install/generate_pnp_build_files.ts

+async function main() {
+  const packageJson = require(path.join(workspacePath, "package.json"));
+  const dependencies = Object.keys(packageJson.dependencies || {});
+  const devDependencies = Object.keys(packageJson.dependencies || {});


Suggested change

const devDependencies = Object.keys(packageJson.dependencies || {});

const devDependencies = Object.keys(packageJson.devDependencies || {});

also: what about peer and optional dependencies?

joscha · 2019-11-25T05:13:25Z

internal/npm_install/process_pnp_file.ts

+ * not the entry_point's directory. PnP assumes they are the same so when bazel creates the
+ * pnp file in a different directory, the pnp files can't resolve the paths.
+ */
+function main() {


Suggested change

function main() {

function main(workspacePath: string) {

joscha · 2019-11-25T05:17:06Z

internal/npm_install/process_pnp_file.ts

+  const pnpFile = fs.readFileSync(pnpPath, "utf8");
+  const patchedPnpFile = pnpFile.replace(
+    "issuerModule ? issuerModule.filename : `${process.cwd()}/`;",
+    `"${process.cwd()}"`


Suggested change

`"${process.cwd()}"`

JSON.stringify(process.cwd())

joscha · 2019-11-25T05:17:40Z

internal/npm_install/process_pnp_file.ts

+
+  const pnpPath = path.join(workspacePath, ".pnp.js");
+  const pnpFile = fs.readFileSync(pnpPath, "utf8");
+  const patchedPnpFile = pnpFile.replace(


can you add some docs here with an example of what this is supposed to match? Also, do we expect to only replace the first mach?

we expect their to only ever be 1 match, this is a hack to force the path we look for PnP modules to be the same as where yarn_install is invoked. If we don't do this the pnp resolver defaults to the path the nodejs binary is invoked, which for bazel is different.

I suppose it's not necessarily only supposed to replace once, so we should do a global replace to make it better

joscha · 2019-11-25T05:18:04Z

internal/npm_install/pre_process_package_json.js

@@ -53,9 +53,13 @@ function main() {
    // Note: there is no equivalent npm functionality to clean out individual packages
    // from the npm cache.
    clearYarnFilePathCaches(pkg);
+    console.log(isPnp(pkg));


Suggested change

console.log(isPnp(pkg));

leftover?

This script is called by bazel as a way to tell the rule that the workspace is a pnp workspace, bazel can only take from stdout, so we need to log this to the console

joscha · 2019-11-25T05:23:14Z

internal/npm_install/generate_pnp_build_files.ts

+  const packageJson = require(path.join(workspacePath, "package.json"));
+  const dependencies = Object.keys(packageJson.dependencies || {});
+  const devDependencies = Object.keys(packageJson.dependencies || {});
+  dependencies.concat(devDependencies).forEach(createBuildFile);


cheap enough, but there is probably no advantage over creating a new combination array and then iterating.

Object.keys(packageJson.dependencies || {}).forEach(createBuildFile); Object.keys(packageJson.devDependencies || {}).forEach(createBuildFile);

will be as expressive but half the LOC.

or if you're keen:

[ 'dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies' ] .forEach(d => Object.keys(packageJson[d] || {}).forEach(createBuildFile));

second one is nice

joscha · 2019-11-26T05:04:02Z

internal/npm_install/generate_pnp_build_files.ts

+  if (!existsSync(path.dirname(p))) {
+    mkdirSync(path.dirname(p), { recursive: true });
+  }
+  return fsp.writeFile(p, content);


this will return a Promise object now. Which is quite confusing because you use the sync methods above.

I suggest at least changing the signature to:
function writeFile(p: string, content: string): Promise<void> {

or even better:

async function writeFile(p: string, content: string): Promise<void> { await fsp.mkdir(path.dirname(p), { recursive: true }); return await fsp.writeFile(p, content); }

yeah, this should be renamed

joscha · 2019-11-26T05:05:58Z

internal/npm_install/generate_pnp_build_files.ts


-  writeFileSync(`${pkg}/BUILD.bazel`, contents);
+  return writeFileSync(`${packagePath}/BUILD.bazel`, contents);


writeFile is not synchronous anymore which makes it return a promise and thus also makes createBuildFile async.

this is intentional, I should be dealing with all the async stuff with Promise.all though I may have missed something

joscha · 2019-11-26T05:07:58Z

internal/npm_install/generate_pnp_build_files.ts

+      "peerDependencies",
+      "optionalDependencies"
+    ].map(d => {
+      Promise.all(


the value of this is never returned up from the map, so if anything gets rejected it will be unhandled. It also means that your outer Promise.all waits for nothing:

Suggested change

Promise.all(

return Promise.all(

joscha · 2019-11-26T05:08:28Z

internal/npm_install/generate_pnp_build_files.ts

+      Promise.all(
+        Object.keys(packageJson[d] || {})
+          .map(createBuildFile)
+          .concat([writeFileSync(`BUILD.bazel`, contents)])


Suggested change

.concat([writeFileSync(`BUILD.bazel`, contents)])

.concat([writeFileSync("BUILD.bazel", contents)])

or

Suggested change

.concat([writeFileSync(`BUILD.bazel`, contents)])

.concat([writeFileSync('BUILD.bazel', contents)])

as you're not using any interpolation.

joscha · 2019-11-26T05:11:02Z

internal/npm_install/pre_process_package_json.js

@@ -53,9 +53,13 @@ function main() {
    // Note: there is no equivalent npm functionality to clean out individual packages
    // from the npm cache.
    clearYarnFilePathCaches(pkg);
+    console.log(JSON.stringify({pnp: isPnp(pkg)}));


Suggested change

console.log(JSON.stringify({pnp: isPnp(pkg)}));

process.stdout.write(JSON.stringify({pnp: isPnp(pkg)}));

makes it more obvious that this is a deliberate print to sdtout.

danjuv · 2019-11-26T06:08:48Z

third_party/github.com/bazel_json/LICENSE

@@ -0,0 +1,20 @@
+Copyright 2019 Erick Johnson <[email protected]>


third party json parser, we can't introduce it as an http_archive since you can't patch the workspace file in a repo

I don't think that fully explains why the vendoring is necessary, but there is an example of this upstream already so I think this is ok:
https://github.com/bazelbuild/rules_nodejs/tree/a13f2b6e2a132954fb63b7a905818cf8a6ceb824/third_party/github.com/bazelbuild/bazel-skylib/rules

danjuv · 2019-11-27T04:11:26Z

internal/node/node.bzl

@@ -200,7 +200,7 @@ def _nodejs_binary_impl(ctx):
    node_tool_files.append(ctx.file._bazel_require_script)

    if not ctx.outputs.templated_args_file:
-        templated_args = ctx.attr.templated_args
+        templated_args = [a for a in ctx.attr.templated_args]


this string list is frozen, so I created a new list so we can mutate it (add the pnp flag if necessary)

uri-canva · 2019-11-28T03:08:20Z

internal/node/node.bzl

@@ -200,7 +200,7 @@ def _nodejs_binary_impl(ctx):
    node_tool_files.append(ctx.file._bazel_require_script)

    if not ctx.outputs.templated_args_file:
-        templated_args = ctx.attr.templated_args
+        templated_args = [a for a in ctx.attr.templated_args]


Try using .to_list(), I'm not 100% sure it works, but if it does, it's nicer than an identity list comprehension.

uri-canva · 2019-11-28T03:10:13Z

internal/node/node.bzl

@@ -226,6 +226,10 @@ def _nodejs_binary_impl(ctx):

    is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS]

+    output_files = [ctx.outputs.loader]


Why is this called output_files? Isn't ctx.file.pnp_file an input file?

uri-canva · 2019-11-28T03:14:17Z

internal/npm_install/npm_install.bzl

@@ -277,6 +310,7 @@ cd "{root}" && "{npm}" {npm_args}
    )
    if result.return_code:
        fail("pre_process_package_json.js failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (result.stdout, result.stderr))
+    is_pnp = json_parse(result.stdout)["pnp"]


Since you can parse json, why not parse package.json directly, rather than going through the script?

Because Starlark doesn't support file i/o as far as I know

Try https://docs.bazel.build/versions/master/skylark/lib/repository_ctx.html#read.

oh nice, makes sense it's exposed on repository rules

uri-canva

Very close! I think if we can read the package.json from starlark then it's good.

danjuv · 2020-01-02T06:34:43Z

see https://canvadev.atlassian.net/browse/DEVF-831 for more details

uri-canva suggested changes Nov 21, 2019

View reviewed changes

uri-canva suggested changes Nov 22, 2019

View reviewed changes

joscha reviewed Nov 25, 2019

View reviewed changes

joscha reviewed Nov 26, 2019

View reviewed changes

danjuv commented Nov 26, 2019

View reviewed changes

danjuv commented Nov 27, 2019

View reviewed changes

danjuv force-pushed the rules_pnp branch from 6a85fe3 to 1b3d79f Compare November 27, 2019 07:23

danjuv added 2 commits November 27, 2019 18:50

PNP rule

b014f92

move node modules post processing into non pnp flow

8167c21

danjuv force-pushed the rules_pnp branch from 1b3d79f to 8167c21 Compare November 28, 2019 02:32

danjuv changed the base branch from master to basedpr November 28, 2019 02:41

uri-canva reviewed Nov 28, 2019

View reviewed changes

uri-canva approved these changes Nov 28, 2019

View reviewed changes

danjuv closed this Jan 2, 2020


		flattenedDeps.forEach(createBuildFile);

		processDependencies(devDependencies, "devDependencies");


		def _create_build_files(repository_ctx, rule_type, node, lock_file):
		]


		const workspacePath = process.argv.slice(2)[0];

		function mkdirp(p: string) {

	async function main() {
	async function main(workspacePath: string) {

	const devDependencies = Object.keys(packageJson.dependencies \|\| {});
	const devDependencies = Object.keys(packageJson.devDependencies \|\| {});


		writeFileSync(`${pkg}/BUILD.bazel`, contents);
		return writeFileSync(`${packagePath}/BUILD.bazel`, contents);

	.concat([writeFileSync(`BUILD.bazel`, contents)])
	.concat([writeFileSync("BUILD.bazel", contents)])

	.concat([writeFileSync(`BUILD.bazel`, contents)])
	.concat([writeFileSync('BUILD.bazel', contents)])

	console.log(JSON.stringify({pnp: isPnp(pkg)}));
	process.stdout.write(JSON.stringify({pnp: isPnp(pkg)}));

		@@ -0,0 +1,20 @@
		Copyright 2019 Erick Johnson <[email protected]>

		@@ -226,6 +226,10 @@ def _nodejs_binary_impl(ctx):

		is_builtin = ctx.attr._node.label.workspace_name in ["nodejs_%s" % p for p in BUILT_IN_NODE_PLATFORMS]

		output_files = [ctx.outputs.loader]

PNP rule #1

PNP rule #1

Conversation

danjuv commented Nov 21, 2019 • edited Loading

uri-canva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uri-canva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danjuv commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uri-canva left a comment

Choose a reason for hiding this comment

danjuv commented Jan 2, 2020

danjuv commented Nov 21, 2019 •

edited

Loading