feat: add build step for consistent artifact generation by jsejcksn · Pull Request #2 · reggi/fortune-cookie

jsejcksn · 2022-08-28T04:20:59Z

I like that you collected these fortune messages and shared them! I always find them fun or funny when I get one.

I noticed a typo while reading them and started to create a PR for a single line fix, but then I kept reading and noticed some more, and saw an opportunity to improve the overall consistency of the collection with a simple build step to generate the other file formats from the text file.

The corrections I made are all overtly obvious grammatical/syntactical ones. I didn't worry about stylistic things or misplaced commas, etc. I know that part of the allure of fortune cookie messages can be the occasional bad translation or strange phrasing, and I generally avoided engaging those cases as well. In case it's not clear what I meant by the previous sentence, here's an example from your collection with a link to someone's take/explanation:

May the warm winds of heaven blow softly upon your sprint.

https://followingfunny.com/2011/10/10/fortune/

In addition to generating other file formats, the build script ensures additional consistency in things like file names, whitespace, a simple deduplication algorithm (which removed several duplicates), etc. You can read the comments in the script to get precise clarity.

Re: ESM: Here's a link to the current LTS version of the Node documentation that addresses JSON import assertions: https://nodejs.org/docs/latest-v16.x/api/esm.html#json-modules

Final notes:

Implementing consistent file naming is a breaking change (as is switching to ESM), and I didn't increment the package version in this PR. I just wanted to mention that in case you decide to accept it.

I also wrote this simple, self-contained React app to review the quotes as I was working on the PR — it works without any build step, using just a local static file http server (I used this one):

viewer.html

<!doctype html>
<html lang="en">

<head>
  <meta charset="utf-8" />
  <meta name="description" content="fortune cookie messages" />
  <meta name="viewport" content="width=device-width, initial-scale=1" />

  <title>fortune cookie messages</title>

  <link rel="preconnect" href="https://fonts.googleapis.com">
  <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
  <link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;600&display=swap" rel="stylesheet">
  <style>
    @import url('https://unpkg.com/sanitize.css@13.0.0/sanitize.css');

    .app {
      font-family: 'Inter', sans-serif;
      font-size: 2rem;
    }

    .fortune-viewer {
      padding: 2rem 2rem;
    }

    .fortune-index {
      display: flex;
      gap: 0.5rem;
    }

    .pagination-button {
      font-size: 2rem;
    }

    .input-index {
      font-size: 2rem;
      width: 5ch;
    }

    .fortune {
      font-size: 4rem;
      font-weight: 600;
    }
  </style>

  <script src="https://unpkg.com/react@18.2.0/umd/react.development.js"></script>
  <script src="https://unpkg.com/react-dom@18.2.0/umd/react-dom.development.js"></script>
  <script src="https://unpkg.com/@babel/standalone@7.18.13/babel.min.js"></script>
  <script type="text/babel" data-type="module" data-presets="env,react">

  const {StrictMode, useEffect, useRef, useState} = React;

  function parseAsNumber (str) {
    const n = Number(str);
    return Number.isNaN(n) ? 0 : n;
  }

  function parseAsInt (str) {
    const n = parseAsNumber(str);
    return Number.isInteger(n) ? n : Math.floor(n);
  }

  function wrapNumber (n, ceil) {
    n %= ceil;
    if (n < 0) n += ceil;
    return n;
  }

  function useJsonValue (url) {
    const ref = useRef({isFirstEffect: true, previous: url});
    const [data, setData] = useState(undefined);
    const [error, setError] = useState(undefined);
    const [loading, setLoading] = useState(true);

    useEffect(() => {
      if (ref.current.isFirstEffect) ref.current.isFirstEffect = false;
      else if (ref.current.previous === url) return;
      else ref.current.previous = url;

      const fetchJson = async () => {
        try {
          setLoading(true);
          const resopnse = await fetch(url);
          if (!resopnse.ok) throw new Error('Response not OK');
          const parsed = await resopnse.json();
          setData(parsed);
          setError(undefined);
        }
        catch (ex) {
          if (!(ex instanceof Error)) {
            setError(new Error('An unexpected error occurred', {cause: ex}));
            return;
          }
          setError(ex);
        }
        finally {
          setLoading(false);
        }
      };

      fetchJson();
    }, [url]);

    return {data, error, loading};
  }

  function Fortune ({text}) {
    return (<p className="fortune">{text}</p>);
  }

  function FortuneViewer ({fortunes}) {
    const [rawValue, setRawValue] = useState('1');
    const [index, setIndex] = useState(0);

    const handleChange = (ev) => {
      let str = ev.target.value;

      if (!str.trim()) {
        setIndex(0);
        setRawValue('1');
        return;
      }

      let parsed = parseAsInt(str) - 1;
      const wrapped = wrapNumber(parsed, fortunes.length);
      setIndex(wrapped);
      if (parsed < 0 || parsed > fortunes.length - 1) str = String(wrapped + 1);
      setRawValue(str);
    };

    return (
      <div className="fortune-viewer">
        <div className="fortune-index">
          <button
            className="pagination-button"
            onClick={() => {
              const nextIndex = wrapNumber(index - 1, fortunes.length);
              setIndex(nextIndex);
              const nextValue = String(nextIndex + 1);
              setRawValue(nextValue);
            }}
          >⬅️</button>
          <button
            className="pagination-button"
            onClick={() => {
              const nextIndex = wrapNumber(index + 1, fortunes.length);
              setIndex(nextIndex);
              const nextValue = String(nextIndex + 1);
              setRawValue(nextValue);
            }}
          >➡️</button>
          <input
            type="number"
            placeholder="fortune index"
            className="input-index"
            onChange={handleChange}
            value={rawValue}
          />
          <div>/ {fortunes.length}</div>
        </div>
        <Fortune text={fortunes[index]} />
      </div>
    );
  }

  function ErrorMessage ({error}) {
    return (<p>{error.toString()}</p>);
  }

  function LoadingMessage () {
    return (<p>Your fortunes are being discovered...</p>);
  }

  function App () {
    const {data, error, loading} = useJsonValue('./fortune-cookies.json');

    const reactNode = loading
      ? <LoadingMessage />
      : error
        ? <ErrorMessage {...{error}} />
        : <FortuneViewer {...{fortunes: data}} />;

    return (
      <div className="app">
        {reactNode}
      </div>
    );
  }

  const reactRoot = ReactDOM.createRoot(document.getElementById('root'));

  reactRoot.render(
    <StrictMode>
      <App />
    </StrictMode>
  );

  </script>
</head>

<body>
  <div id="root"></div>
</body>

</html>

I hope you find this PR useful. Thanks again for sharing your collection.

reggi · 2022-08-28T06:32:01Z

Hah! @jsejcksn thanks so much for this, and thanks for your help over on stack-overflow!

Thoughts:

I found the archive of physical fortune cookies which this is based on, I still have it ❤️🥠
One issue is I added more to the pile, so I'd like to update the list and I have to go through them.
I remember adding duplicates because I have duplicates! I thought it was interesting to include that data somehow.
I'd like to re-enter and check the spelling of the ones you changed, If indeed they are spelled wrong on the fortune then we may want to keep them wrong in here.
I have a new list I just hand-entered going through them in a google doc
I see some of the ones you added are in this list! Coincidentally I have some of those fortunes!
~~I'd be interested in a deno cut but as a static array in a ts file, i'd consider the same for node with a js file as well.~~
You can add your site as a gh-pages to this repo if you want, then the link to the repo could go to it?

I am struggling with the integrity of the fortunes. As in adding new ones that I don't physically have in my possession. I feel like the fort knox of fortune cookies, as in every one in the repo is backed by a piece of paper. Gold as to fortune paper, as fortune is to cash? Or when the dollar was backed by gold. Anyway. I'd be down to add a CSV for third-party contributions but with some real-world backing, perhaps a photo on twitter or instagram with a real non-bot user? Or mail me your fortunes (lol)! What I'm afraid of is people making PR's of nonsense fortunes they've heard about online cluttering this repo up.

Here's an example of one I found that is authored that way:

reggi · 2022-08-28T07:24:23Z

reggi · 2022-08-28T07:34:08Z

reggi · 2022-08-28T07:38:39Z

You definitely found some good typos! But i'd like to keep the ones as they are if they are written as-is.

jsejcksn · 2022-08-28T09:14:03Z

@reggi Thanks for sharing those thoughts and explaining more.

I found the archive of physical fortune cookies which this is based on, I still have it ❤️🥠

Reading this was simultaneously somewhat expected, yet still a surprise to me.

I see some of the ones you added are in this list! Coincidentally I have some of those fortunes!

I actually didn't contribute any: During the cleanup, I noticed that the JSON file had some additional fortunes appended to the end of the list that weren't in the text file, and I simply copied them to the text file.

You can add your site as a gh-pages to this repo if you want, then the link to the repo could go to it?

Thanks for your welcoming suggestion. I think that can be addressed in a separate PR if you want to include it — especially if it involves another branch. I shared it here simply to give you insight into my process (and to share an artifact of that in case you found it useful).

Re: the Google spreadsheet link:

I couldn't access it when I tried. Perhaps you can double-check that its access level is set to public if you intended for me and other viewers of this PR to be able to view it.

Re: no. 2 through no. 5 and the rest of your message:

When I found the repo, the personal nature of the collection wasn't clear to me — I didn't realize that it was intended as a digital copy of the verbatim strings of the messages from your personal preservation, and that you "[felt] like the fort knox of fortune cookies, as in every one in the repo is backed by a piece of paper." I also didn't assume that you mis-typed anything when transcribing. And with those in mind, I hope it makes sense to hear that I wouldn't have even begun the PR had I understood this aspect. Since this is very personal to you, I completely understand wanting to maintain the accuracy of the details in that way. I think somehow including digital evidence (like those photos) in a directory of the repo will help communicate the nature of what you described.

So — that said — are there parts of the PR that you want? My effort in this was to help make it more usable by others and easier for you to maintain: just keep updating the text file and generate the other artifacts.

Toward the former concern: if you want to keep duplicates, perhaps that part of the build step can be omitted and documented as an exported function that can be used by consumers who don't want the duplicates.

Do you want to just update the text file and regenerate the other file formats, keeping the build script, ESM, etc.? How would you like for this PR to proceed?

reggi · 2022-08-28T19:57:47Z

Ah! Ok good learnings here.

Can you split this PR into two one with just the build.mjs file and another with the fortune-cookies.txt?

I am worried about dropping cjs support for older versions of node, any ideas on how to support both? I'm also curious if the package main file could just be the json file could be support for both CJS and MJS like that?

I agree with removing dupes for release I can add a build step for that.

reggi · 2022-08-28T19:59:07Z

I'll try setting up a github action to build the files and publish, so no need to include the built files here.

jsejcksn · 2022-08-29T19:53:06Z

@reggi

Can you split this PR into two one with just the build.mjs file and another with the fortune-cookies.txt?

Of course! But before I do that, I'd like to respond to some of what you said:

I am worried about dropping cjs support for older versions of node, any ideas on how to support both?

Are you trying to support unsupported versions of Node? All versions of Node that aren't already past end-of-life have support for ESM. CommonJS was very useful when there was no standardized module system in JavaScript, but now it's a relic. If you are committed to supporting those ancient versions of Node (and a non-standard module system), there's the package entry points feature.

I'm also curious if the package main file could just be the json file could be support for both CJS and MJS like that?

Importing of JSON modules in ESM requires the use of an import assertion which is a stage 3 proposal, (it's considered experimental until finalized because there's a [microscopically small] possibility that the syntax/behavior could still change before then), so I don't recommend providing a JSON file as the entrypoint for ESM consumers right now. Beyond any potential stability concerns, it would require consumers to know that your entrypoint is JSON ahead of time and to actually use the import assertion syntax when importing your (JSON) package:

import fortunes from 'fortune-cookie' assert {type: 'json'};

IMO, this is suboptimal DX for the consumer because of the increased cognitive load, and I recommend providing a JS module entrypoint that handles this for them "behind the scenes" — see my comments in index.mjs.

I'll try setting up a github action to build the files and publish, so no need to include the built files here.

I'm not sure if you meant that you are thinking about removing the generated files from the repo. If so, here's a (perhaps unconventional) opinion for perspective:

I think there's great value in keeping those build artifacts in the repo (in a directory or even if in a different branch from the source): not everyone has the same experience level in tooling (so not everyone can build — and, even if they could, it's such a chore when you just need a single file), so having equal access to static artifacts in the same way as static source is a win for accessibility.

It's very annoying to me when I need access to some built artifact in an npm package (e.g. a generated wasm module in a non-Node environment like Deno, etc.) and I can't get it from GitHub because it doesn't exist in the repo (or it's only included as one asset in a binary tarball/zip archive in a release), so I have to either download/extract/cleanup (which is even more cumbersome if it's needed as a resource provided by an automated network request) or go find it at some third-party CDN (e.g. unpkg), relying on its availability. (end of rant)

So...

Are you positive that you need to support CJS?
How do you plan to handle build? (so I know how to restructure this PR and create the next one)

and cleanup duplicate npm keywords

jsejcksn · 2022-09-01T00:54:00Z

@reggi While awaiting your reply, I went ahead and rebased this PR: now it only addresses the build step and generation of artifacts (the other file formats besides the source text file — ES module, JSON, Markdown).

I also added JSDoc type annotations in the build script during the refactor, and now it only deduplicates the fortunes for the generated artifacts and not the text source (per our discussion above).

Hopefully that helps reduce complexity here by separating concerns — and if you decide to consider the other changes, those can be handled in separate PRs.

jsejcksn added 4 commits August 31, 2022 18:57

fix: add missing fortunes to txt source from JSON

c8e5807

feat: add build script

d5f5c5b

and cleanup duplicate npm keywords

build: generate artifacts

bb15788

fix: update JSON file path in require statement

df3d9b8

jsejcksn force-pushed the refactor/consistency branch from 6e39987 to df3d9b8 Compare September 1, 2022 00:41

jsejcksn changed the title ~~refactor: consistency~~ feat: add build step for consistent artifact generation Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add build step for consistent artifact generation#2

feat: add build step for consistent artifact generation#2
jsejcksn wants to merge 4 commits intoreggi:masterfrom
jsejcksn:refactor/consistency

jsejcksn commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022 •

edited

Loading

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022 •

edited

Loading

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

jsejcksn commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

jsejcksn commented Aug 29, 2022 •

edited

Loading

Uh oh!

jsejcksn commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jsejcksn commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

jsejcksn commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

reggi commented Aug 28, 2022

Uh oh!

jsejcksn commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsejcksn commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

reggi commented Aug 28, 2022 •

edited

Loading

reggi commented Aug 28, 2022 •

edited

Loading

jsejcksn commented Aug 29, 2022 •

edited

Loading