Skip to content

Conversation

@pavellishin
Copy link
Contributor

When a <guid> element is given, it's considered to be an RSS feed item's permalink by default (see the spec: https://cyber.harvard.edu/rss/rss.html#ltguidgtSubelementOfLtitemgt)

This is not always the preferred behavior, since some RSS clients will treat the guid as the link when attempting to open the item in a browser, and if this is not a valid URL, this will fail. (For example, Bluesky posts' uri is not a valid URL, but instead looks like at://did:plc:abc123/app.bsky.feed.post/def456).

This PR changes the behavior to explicitly specify isPermaLink="false" if a guid or an id is given for an item, but will set it to true if only the link is given.

This addresses issue #212.

@pavellishin
Copy link
Contributor Author

Side note: my personal preference would be for the sampleFeed to be re-generated for every test, instead of every test appending yet more items to it - otherwise, inserting a test in the middle will cause the test suite to fail because every subsequent test will be modified, and no longer match the snapshot.

I also had to make some local changes to be able to build & run tests; I have not committed those here.

@jpmonette
Copy link
Owner

@pavellishin Which changes have you had to make locally to make this work?

Thanks for the contribution.

@jpmonette jpmonette merged commit e9807fe into jpmonette:master May 27, 2025
@jpmonette
Copy link
Owner

Seems like this change broke the tests:
a0389cb

I've fixed it & added a new Github Action to run tests, this should ensure it doesn't happen again.

@pavellishin
Copy link
Contributor Author

@pavellishin Which changes have you had to make locally to make this work?

Thanks for the contribution.

Oof, I think I cloned the repo to a temporary directory and it's gone now. I'll see if I can reproduce my issues and get back to you.

I don't have the best recollection, but some of the changes in #215 smell familiar...

@pavellishin
Copy link
Contributor Author

Ok, I just re-cloned it, and ran pnpm i and pnpm test, and it worked fine.

I do think that's due to the changes in #215; if I check out the commit before my PR (commit:1a29418), and run pnpm test, I start getting familiar-sounding errors:

# pnpm test

> feed@5.0.1 test /Users/pavel/personal/projects/feed
> jest --silent

ReferenceError: module is not defined
    at file:///Users/pavel/personal/projects/feed/jest.config.js:1:1
    at ModuleJobSync.runSync (node:internal/modules/esm/module_job:400:35)
    at ModuleLoader.importSyncForRequire (node:internal/modules/esm/loader:427:47)
    at loadESMFromCJS (node:internal/modules/cjs/loader:1565:24)
    at Module._compile (node:internal/modules/cjs/loader:1716:5)
    at Object..js (node:internal/modules/cjs/loader:1899:10)
    at Module.load (node:internal/modules/cjs/loader:1469:32)
    at Function._load (node:internal/modules/cjs/loader:1286:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
 ELIFECYCLE  Test failed. See above for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants