Close #1014, ALKiP notifications by plocket · Pull Request #1041 · SuffolkLITLab/ALKiln

plocket · 2026-02-02T14:20:46Z

In this PR, I have:

Added tests for any new features or bug fixes (if relevant)
Added my changes to the CHANGELOG.md at the top, under the "Unreleased" section
Ensured issues that this PR closes will be automatically closed

TODO:

Implement newer changes in https://github.com/SuffolkLITLab/ALKiln/tree/1014_breaking_news. Most easily seen in this repo's network graph. Leave out the changes to do with file generating.

Reason for this PR

[Early PR for quick top-level sanity check review, failing tests are fine for now.]
Summary: Add notifications to ALKilnInThePlayground (ALKiP) for ALKiln changes
Details:
This is a testing framework. One of the environments in which it runs is on a test author's own server. ALKiP is a package of mine that helps authors runs these tests on their server. I work hard to ensure backwards compatibility, but it can be useful to keep both up to date with the other and sometimes to notify authors of other changes to their server that might affect how ALKiP is running. It's hard for authors to tell which versions match up with each other and when to update in general.

This ALKiln PR generates HTML that ALKiP can use to add notifications about relevant changes to ALKiln, to the author's server, or about whatever else ALKiln thinks is helpful. We will use notifications sparingly.

Demo of the visual appearance of a notification. Visual preview:

Links to any solved or related issues

Closes #1014

Any manual testing I have done to ensure my PR is working

Publishing to npm and testing on ALKiP

any announcements new ALKiln versions have even if ALKilnInThePlayground isn't itself updated. Closes #1014 Co-authored-by: will-morningstar <mxwillmorningstar@gmail.com>

…n into 1014_breaking_news_2

tobynorth

Looks good to me! Didn't find any major issues. One high-level, almost certainly out of scope question for my own understanding: it seems like the need for this PR arose out of ALKiln and ALKiP being separate packages -- why is that? (would you ever want to install ALKiP without wanting to install ALKiln?)

And a general question: how are all the logging changes (artifacts.js, validate.js, Log.js) and path changes (files.js, session_vars.js, set_sources_path.js, scope.js) related to the new notifications feature? For clarity, I'd suggest either putting something about these changes in the PR description or moving them to separate PR(s).

tobynorth · 2026-02-03T21:35:03Z

 const argv = require(`minimist`)( process.argv.slice(2) );
-const artifacts_path = files.make_artifacts_folder( argv.path );
+
+const artifacts_path = files.make_artifacts_folder();


nit: argv is no longer used -- consider either cleaning it up or (if this was accidental) restoring it as an arg to make_artifacts_folder

Great point, thank you!

I realize you have no context, but I'm going to think about this "out loud", so if you do have thoughts I'd appreciate them. tl;dr: ~~I think I could bring it back to add flexibility without any particular downside.~~ [I think I should remove argv considering how session_vars finds the config path now, though I'm still not sure I'm a fan.]

If I recall correctly, I had meant to remove it. I think the caller is now meant to make sure the root of the command is where the artifacts folder should be. In ALKiP (a project that uses this lib), for example, the cwd argument for subprocess ensures the root is in /tmp. I'm actually not sure I like it as the behavior is a bit hidden. I remember having trouble wrangling the root of the path for ALKiP for some reason, though, and landing on this solution. ~~Seems to me like I could have both without a problem.~~ [Going to keep using cwd for now, though, since session_vars is strictly using cwd with no other arguments. It makes sense I suppose. We'd otherwise have to manage an additional env var. See session_vars.get_runtime_config_path().]

[Note to self: Worth adding some of these notes to the docs.]

tobynorth · 2026-02-05T22:12:05Z

+  // Save Scenario data in Scenario folder
+  // '@alkiln @generated'
+  // console.log( JSON.stringify( scenario ) );


question/nit: did you mean to commit these comments?

Nice catch!

Suggested change

// Save Scenario data in Scenario folder

// '@alkiln @generated'

// console.log( JSON.stringify( scenario ) );

tobynorth · 2026-02-05T22:16:04Z

 {
  "name": "@suffolklitlab/alkiln",
-  "version": "5.15.4",
+  "version": "5.15.0-feat-news-17",


question (non-blocking): just to satisfy my own curiosity, what's going on with the version number here? (in particular, why did the patch version number drop from 4 to 0?)

No problem: I just manage branch versions based off of their base branch. When I first diverged from the default branch, the version was 5.15.0, that's all. I'm curious to hear if you've seen other methods!

tobynorth · 2026-02-05T22:56:44Z

  scope.paths.scenario_report = `${ scope.paths.scenario }/report.txt`;
+  // If it's a generated Scenario, save the `.feature` file in here
+  // GENERATED_FILES_FOLDER_NAME
+  if ( scenario.gherkinDocument.uri.includes("_alkiln_generated") ) {


nit: _alkiln_generated is a magic string, ideally would be defined in a more maintainable location. (Where did it come from, anyway? I don't see any other references to it in this PR)

Good catch here too. The name is already in globals.js and I should be using that instead. I made a variable for it and everything! Looks like I don't need a separate variable for that atm anyway.

Suggested change

if ( scenario.gherkinDocument.uri.includes("_alkiln_generated") ) {

if ( scenario.gherkinDocument.uri.includes(globals.generated_files_folder_name) ) {

tobynorth · 2026-02-05T23:02:46Z

+   * Note that the function prints the value to the console before returning the
+   *     value. This is the only way ALKilnInThePlayground can get the value.


question: why is this? Seems cleaner to use a temp file; is that not feasible for some reason?

tobynorth · 2026-02-06T18:09:14Z

+    for ( let subhead of shuffled_subheads ) {
+
+      if ( subhead.includes(`!`) && used_exclamation ) { continue; }
+      if ( subhead.includes(`?`) && used_question ) { continue; }
+      if ( subhead.includes(`"`) && used_quotes ) { continue; }
+      if ( subheads_used[ subhead ]) { continue; }
+
+      if ( subhead.includes(`!`)) { used_exclamation = true; }
+      if ( subhead.includes(`?`)) { used_question = true; }
+      if ( subhead.includes(`"`)) { used_quotes = true; }
+
+      final_subhead = subhead;
+      subheads_used[ subhead ] = true;
+    }


suggestion: it feels like it would be a little cleaner to refactor this into get_shuffled_subheads by just ensuring that a max of 1 subhead with each checked char (and no duplicate subheads) are added to the return array. Then in this function, you could just pop or shift an element off the array to pass to get_one_articles_parts

tobynorth · 2026-02-06T18:20:22Z

+  let html_paragraphs = [];
+  for ( let one_paragraph of paragraphs ) {
+    html_paragraphs.push(`<p class="body_copy">${ one_paragraph }</p>`);
+  }
+  return html_paragraphs;


suggestion: this could be simplified to:

Suggested change

let html_paragraphs = [];

for ( let one_paragraph of paragraphs ) {

html_paragraphs.push(`<p class="body_copy">${ one_paragraph }</p>`);

}

return html_paragraphs;

return paragraphs.map(paragraph => `<p class="body_copy">${ paragraph }</p>`);

tobynorth · 2026-02-06T18:33:48Z

+  if ( typeof list_or_str === `string` ) {
+    indent_str = `  `.repeat( indent );
+    return [ indent_str + list_or_str ];
+  }
+
+  let flattened = [];
+  for ( let nested_str_or_list of list_or_str ) {
+    let result = indent_and_flatten_html_list(
+      nested_str_or_list,
+      indent + 1
+    );
+    // // .concat() feels evil because it accepts a string as well
+    // flattened = flattened.concat( result );
+    flattened.push( ...result );
+  }
+
+  return flattened;


suggestion: I think this could be simplified to:

Suggested change

if ( typeof list_or_str === `string` ) {

indent_str = ` `.repeat( indent );

return [ indent_str + list_or_str ];

}

let flattened = [];

for ( let nested_str_or_list of list_or_str ) {

let result = indent_and_flatten_html_list(

nested_str_or_list,

indent + 1

);

// // .concat() feels evil because it accepts a string as well

// flattened = flattened.concat( result );

flattened.push( ...result );

}

return flattened;

return list_or_str.flatMap(nested_str_or_list =>

Array.isArray(nested_str_or_list)

? indent_and_flatten_html_list(nested_str_or_list, indent + 1)

: ` `.repeat(indent) + nested_str_or_list

);

tobynorth · 2026-02-06T18:41:36Z

+}
+
+
+function get_semver_parts( version_str ) {


suggestion: Consider using a 3rd party library to replace all your semver-related functions if you haven't already (example). Since this is neither related to the core ALKiln logic nor (it seems) something ALKiln needs to go outside the semver spec for, probably better to let someone else maintain it

tobynorth · 2026-02-06T18:46:49Z

+    // If we switch to module (import) instead of common (require), use
+    // https://nodejs.org/api/esm.html#importmetafilename for file path
+    let most_recent_file_sha = execSync(
+      `git rev-list -1 HEAD -- "${ __filename }"`


question: this is running on users' servers, right? Is it safe to assume they have git installed and have cloned the ALKiln repo locally?...

plocket · 2026-04-07T20:40:47Z

+const log = new Log({ path: artifacts_path, context: `artifacts` });
+log.info({ code: `ALK0284`, context: `artifacts`,},
+  `Created the artifacts folder at "${ artifacts_path }"`
+);
+
+// This is just for artifacts, not any other config values. Leave the config
+// Project name value as it is. Local developers find it useful to re-use it.


What does "This" mean, Past Self? I think it meant that saving the artifacts path name here is unique in some way. Maybe because it's updated with every run of the tests. If so, we should write documentation about which config values get changed and when they get changed. Suggestion:

Changing config values:

Artifacts folder: name is different with each test run since artifacts are unique to each test run.

Project name: the env vars determine the Project name. Local devs often re-use the same Project. Other environments, like GitHub or ALKiP are one-use and manage their own Project name value.

Remove comment from code here:

Suggested change

const log = new Log({ path: artifacts_path, context: `artifacts` });

log.info({ code: `ALK0284`, context: `artifacts`,},

`Created the artifacts folder at "${ artifacts_path }"`

);

// This is just for artifacts, not any other config values. Leave the config

// Project name value as it is. Local developers find it useful to re-use it.

const log = new Log({ path: artifacts_path, context: `artifacts` });

log.info({ code: `ALK0284`, context: `artifacts`,},

`Created the artifacts folder at "${ artifacts_path }"`

);

plocket · 2026-04-07T21:13:57Z

  // Store path name for this Scenario's individualized report
  scope.paths.scenario_report = `${ scope.paths.scenario }/report.txt`;
+  // If it's a generated Scenario, save the `.feature` file in here
+  // GENERATED_FILES_FOLDER_NAME


Suggested change

// GENERATED_FILES_FOLDER_NAME

plocket · 2026-04-07T21:14:13Z

+
+const GENERATED_FILES_FOLDER_NAME = globals.generated_files_folder_name;


Suggested change

const GENERATED_FILES_FOLDER_NAME = globals.generated_files_folder_name;

plocket · 2026-04-07T21:29:31Z

Thanks for the excellent and detailed review! I've finally started in on this, though it'll stay here a while longer (see below).

And a general question: how are all the logging changes (artifacts.js, validate.js, Log.js) and path changes (files.js, session_vars.js, set_sources_path.js, scope.js) related to the new notifications feature? For clarity, I'd suggest either putting something about these changes in the PR description or moving them to separate PR(s).

Good insticts! The logging and path changes were indeed meant to be in a separate PR and I'll move them there and deal with those first before addressing the notification feature, so this PR is going to end up sitting on the back burner a little while longer unfortunately. And will probably run into merge conflicts unless I make a whole new PR, which I won't because I don't want to lose this awesome review. Some days you can't have it both ways ¯\_(ツ)_/¯

One high-level, almost certainly out of scope question for my own understanding: it seems like the need for this PR arose out of ALKiln and ALKiP being separate packages -- why is that? (would you ever want to install ALKiP without wanting to install ALKiln?)

I think it's understandable without too much further context. There are 4 environments in which ALKiln can run and ALKiP is just one of those. As such, though you'd never want to install ALKiP without ALKiln, you may very well want to install ALKiln without ALKiP. At least, we have some users who do that.

plocket and others added 16 commits June 14, 2025 22:21

Add "Breaking news" feature so ALKilnInThePlayground can show

c49cb80

any announcements new ALKiln versions have even if ALKilnInThePlayground isn't itself updated. Closes #1014 Co-authored-by: will-morningstar <mxwillmorningstar@gmail.com>

Debug ALKiP news

6346c16

Add logs to help with ALKiP development

413aad8

Remove erroring code so we can see what the logs say

0589d1f

Use cwd when creating the path to make files

5e3946a

Use cwd liberally to get a dynamic path to root (for ALKiP)

1050612

Debug node can't find feature files after already finding them ourselves

d457454

Use __dirname in readFileSync to (supposedly) get correct relative path

5c72d3b

Remove *.feature (filename glob) from what is supposed to be a path glob

7cb08ff

Update paths to pass local tests, tweak some names

eae2510

Update paths to pass local tests, tweak some names

6b362a5

Generate html that shows ALKiln notifications ("breaking news")

ff22c9c

Update paths to pass local tests, tweak some names

d56625d

Generate html that shows ALKiln notifications ("breaking news")

9e2f223

Merge branch '1014_breaking_news_2' of github.com:SuffolkLITLab/ALKil…

a506590

…n into 1014_breaking_news_2

Bump experimental version after merging v5

8f34462

plocket requested a review from BryceStevenWilley as a code owner February 2, 2026 14:20

Merge branch 'v5' and bump experimental version

067d26b

plocket marked this pull request as draft February 2, 2026 14:27

tobynorth approved these changes Feb 9, 2026

View reviewed changes

plocket commented Apr 7, 2026

View reviewed changes

plocket mentioned this pull request Apr 19, 2026

Support cwd as well as the base for file paths to integrate with ALKiP changes, #1048

Open

3 tasks

plocket changed the title ~~Close #1014, ALKiP notifications~~ Close #1049, ALKiP notifications Apr 19, 2026

plocket changed the title ~~Close #1049, ALKiP notifications~~ Close #1014, ALKiP notifications Apr 19, 2026

	// Save Scenario data in Scenario folder
	// '@alkiln @generated'
	// console.log( JSON.stringify( scenario ) );

	if ( scenario.gherkinDocument.uri.includes("_alkiln_generated") ) {
	if ( scenario.gherkinDocument.uri.includes(globals.generated_files_folder_name) ) {

		* Note that the function prints the value to the console before returning the
		* value. This is the only way ALKilnInThePlayground can get the value.


		const GENERATED_FILES_FOLDER_NAME = globals.generated_files_folder_name;

Conversation

plocket commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for this PR

Links to any solved or related issues

Any manual testing I have done to ensure my PR is working

Uh oh!

tobynorth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plocket Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

plocket commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

plocket commented Feb 2, 2026 •

edited

Loading

plocket Apr 7, 2026 •

edited

Loading