feat(builtin): support for substitutions #1664

thesayyn · 2020-02-23T16:25:55Z

PR Checklist

Please check if your PR fulfills the following requirements:

[ X] Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
[ X ] Feature (please, look at the "Scope of the project" section in the README.md file)
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
Other... Please describe:

What is the current behavior?

Issue Number: #1621

What is the new behavior?

Does this PR introduce a breaking change?

Yes
[ X ] No

Other information

thesayyn · 2020-02-24T07:32:29Z

//internal/pkg_web/test3:test is failing. I'm gonna fix it asap.

internal/pkg_web/pkg_web.bzl

alexeagle

Thanks for contributing!

alexeagle · 2020-02-26T16:51:25Z

internal/pkg_web/test3/BUILD.bazel

@@ -0,0 +1,26 @@
+load("@build_bazel_rules_nodejs//:index.bzl", "pkg_web")


"test3" isn't a descriptive name, can you make it "substitutions" or similar?

alexeagle · 2020-02-26T16:52:23Z

internal/pkg_web/test3/spec.js

+describe('pkg_web', () => {
+  it('should match the golden file', () => {
+    const output = 'build_bazel_rules_nodejs/internal/pkg_web/test/pkg/index.html';
+    const golden = 'build_bazel_rules_nodejs/internal/pkg_web/test/index_golden.html_';


use the golden_file_test rule instead

alexeagle · 2020-02-26T16:53:37Z

internal/pkg_web/pkg_web.bzl

@@ -48,12 +51,15 @@ def move_files(output_name, files, action_factory, var, assembler, root_paths):
    www_dir = action_factory.declare_directory(output_name)
    args = action_factory.args()
    args.add(www_dir.path)
+    args.add(version_file.path)


I think this needs some conditional logic because the version_file isn't always present?

I think we need to detect use of --stamp like pkg_npm does stamp = ctx.attr.node_context_data[NodeContextInfo].stamp

as far as I know, the version file is always present but the content changes regarding to the --workspace_status_command. Since we have a logic in bundler which checks for the presence of the keys, we'll be okay I guess.

As far as I understand the rule has to obey the --stamp parameter which is stored in the node_context helper rule. I should make this work that way.

alexeagle · 2020-02-26T16:55:35Z

internal/pkg_web/assembler_spec.js

+
+    it('should replace contents with dynamic text from volatile file', () => {
+      assembler.main(
+          [outdir, volatilePath, '{"content":"{TEST_KEY}"}', '--assets', 'path/to/thing1.txt']);


we should make the API match with pkg_npm. Over there we don't let you grab arbitrary keys from the version file, instead we know the key that indicates the version and the replace_with_version attribute which says what placeholder is used.

I suspect you prefer this more general implementation? but I think it's more important to be consistent.

Yes. I think we should permit the users to replace the contents with the arbitrary keys from the version file. It would be more useful this way. Since we have a key check in bundler, the user would get an error if the key doesn't exist in the version file.

In addition, this approach is more general and gives you the ability to replace "placeholder texts" with arbitrary keys from the version file.

For instance, In one of our projects, we are replacing "BUILD_SCM_HASH" with the real hash along with the "BUILD_SCM_VERSION" key and it is not possible to accomplish the same thing with the pkg_npm rule.

Perhaps provide both APIs? The replace_with_version attr on pkg_web would provide a consistent API surface for both rules, and pkg_npm would change to support substitutions from workspace_status?

update, I'm deprecating replace_with_version so let's just do it your way :)

mattem · 2020-06-23T01:14:16Z

internal/pkg_web/assembler.js

+    if (substituteWith.match(/^{.*?}$/)) {
+      substituteWith = substituteWith.replace(/^{(.*?)}$/, '$1');
+      if (!stampMap[substituteWith]) {
+        throw new Error(`Could not find ${substituteWith} key in volatile-status file.`);


I'm not sure about throwing this error here - if the user is replacing 0.0.0-PLACEHOLDER as the version string for regular builds with a key from --workspace_status_command which is tied to --stamp, this will throw when not running a stamped build

When I tie this with the version file from node context, which only appears when --stamp option is present, this would be long gone. So I thought it would be better if we explicitly check for the keys when the user trying to stamp builds with --stamp option present.

mattem · 2020-06-23T01:15:35Z

internal/node/_node_bin/node

@@ -0,0 +1,9 @@
+#!/bin/bash


Revert this file?

This happened automatically when I was trying to commit the changes.

Sure, I'll try to revert it.

mattem · 2020-06-23T01:25:02Z

internal/pkg_web/assembler_spec.js

+
+    it('should replace contents with dynamic text from volatile file', () => {
+      assembler.main(
+          [outdir, volatilePath, '{"content":"{TEST_KEY}"}', '--assets', 'path/to/thing1.txt']);


Perhaps provide both APIs? The replace_with_version attr on pkg_web would provide a consistent API surface for both rules, and pkg_npm would change to support substitutions from workspace_status?

mattem · 2020-06-23T01:27:16Z

internal/pkg_web/golden_test/BUILD.bazel

+        "index.js",
+    ],
+    substitutions = {
+        "THIS_SHOULD_BE_REPLACED": "THIS_SHOULD_BE_REPLACED_WITH_THIS",


nit: I know this is a substitutions, but at first I didn't see the different between the strings after the build as only the suffix changed 😄
Perhaps provide a real world example of a replacement here, as this is mostly centered around version replacement.

I agree, a bit confusing. we can add an example that replaces with an arbitrary key from the volatile file therefore we would have tests and examples for both use cases.

alexeagle · 2020-08-31T15:57:56Z

Hey I'm finally picking up stamping again, I'll get this merged shortly...

thesayyn · 2020-08-31T16:22:24Z

I had no time to work on this and there is still some work to do. Maybe I could get them done by Sunday.

alexeagle · 2020-08-31T20:45:44Z

no worries @thesayyn I have time today and want to make this consistent with other stamping changes I just landed. I'll push a fixup commit

googlebot · 2020-08-31T20:53:42Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

alexeagle · 2020-08-31T20:53:58Z

@googlebot I consent.

googlebot · 2020-08-31T20:54:04Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

thesayyn requested review from alexeagle, gregmagolan and soldair as code owners February 23, 2020 16:25

googlebot added the cla: yes label Feb 23, 2020

thesayyn commented Feb 24, 2020

View reviewed changes

internal/pkg_web/pkg_web.bzl Outdated Show resolved Hide resolved

alexeagle reviewed Feb 26, 2020

View reviewed changes

thesayyn requested a review from alexeagle March 20, 2020 16:13

thesayyn mentioned this pull request Jun 22, 2020

pkg_npm should support stamping with arbitrary stable/volatile keys #1694

Closed

mattem reviewed Jun 23, 2020

View reviewed changes

thesayyn added 2 commits August 31, 2020 13:49

feat(builtin): support for substitutions

c3c3e57

fix: use golden_file_test instead

a10b409

thesayyn requested review from alan-agius4 and jbedard as code owners August 31, 2020 20:53

alexeagle changed the base branch from master to stable August 31, 2020 20:53

googlebot added cla: no and removed cla: yes labels Aug 31, 2020

googlebot added cla: yes and removed cla: no labels Aug 31, 2020

alexeagle approved these changes Aug 31, 2020

View reviewed changes

refactor: cleanup pkg_web stamping feature

c830806

alexeagle merged commit 421280a into bazel-contrib:stable Aug 31, 2020

alexeagle mentioned this pull request Aug 31, 2020

pkg_web: Ability to stamp outputs. #1621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(builtin): support for substitutions #1664

feat(builtin): support for substitutions #1664

thesayyn commented Feb 23, 2020

thesayyn commented Feb 24, 2020

alexeagle left a comment

alexeagle Feb 26, 2020

thesayyn Mar 20, 2020

alexeagle Feb 26, 2020

thesayyn Mar 20, 2020

alexeagle Feb 26, 2020

thesayyn Feb 28, 2020 •

edited

Loading

thesayyn Mar 22, 2020 •

edited

Loading

alexeagle Feb 26, 2020

thesayyn Feb 28, 2020

thesayyn Mar 20, 2020 •

edited

Loading

mattem Jun 23, 2020

alexeagle Aug 31, 2020

mattem Jun 23, 2020

thesayyn Jun 23, 2020

mattem Jun 23, 2020

thesayyn Jun 23, 2020

thesayyn Jun 23, 2020

mattem Jun 23, 2020

mattem Jun 23, 2020

thesayyn Jun 23, 2020

alexeagle commented Aug 31, 2020

thesayyn commented Aug 31, 2020

alexeagle commented Aug 31, 2020

googlebot commented Aug 31, 2020

alexeagle commented Aug 31, 2020

googlebot commented Aug 31, 2020

		@@ -0,0 +1,26 @@
		load("@build_bazel_rules_nodejs//:index.bzl", "pkg_web")

feat(builtin): support for substitutions #1664

feat(builtin): support for substitutions #1664

Conversation

thesayyn commented Feb 23, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

thesayyn commented Feb 24, 2020

alexeagle 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

thesayyn Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

thesayyn Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesayyn Mar 20, 2020 • edited Loading

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

alexeagle commented Aug 31, 2020

thesayyn commented Aug 31, 2020

alexeagle commented Aug 31, 2020

googlebot commented Aug 31, 2020

alexeagle commented Aug 31, 2020

googlebot commented Aug 31, 2020

thesayyn Feb 28, 2020 •

edited

Loading

thesayyn Mar 22, 2020 •

edited

Loading

thesayyn Mar 20, 2020 •

edited

Loading