Skip to content

Conversation

@FSVetaz
Copy link
Contributor

@FSVetaz FSVetaz commented Oct 13, 2025

In the last pr, the master build failed because eslint-config-frontier-react updated after the pr was made, but before it was merged, and that update included a change to the test.

Add package-lock.json to see changes better and not have a pr's checks be possibly different than the master's checks due to deps changing things between a pr creation and merge.

@fs/eslint-config-frontier-react@11.4.2 turned off you-dont-need-lodash-underscore/clone-deep and that made the test fail on master.

To-Dos

  • Run tests (part of pre-push hook)
  • Update demo and tests, if linting configuration is being changed
  • Update documentation & README
  • Increment package.json version

In the last pr, the master build failed because eslint-config-frontier-react updated after the pr was made, but before it was merged, and that update included a change to the test.

Add package-lock.json to see changes better and not have a pr's checks be possibly different than the master's checks due to deps changing things between a pr creation and merge.

@fs/eslint-config-frontier-react@11.4.2 turned off you-dont-need-lodash-underscore/clone-deep and that made the test fail on master.
@FSVetaz FSVetaz requested a review from a team as a code owner October 13, 2025 15:37
Copy link
Collaborator

@skye2k2 skye2k2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason a package.lock was left off originally was to avoid being unnecessarily limiting, so that consumers could passively get updates from tertiary dependencies, such as eslint-config-frontier-react without requiring constant consuming updates and releases here for changes that really don't tend to matter all that much. We have these tests in place so that we can know what changed between releases and could clean up our configuration if necessary, because frontier does not provide that level of insight in their releases. Not because we want to gatekeep.

This is the only time in the six-year history of this linting dependency bundler that we have been impacted by rapidly-changing dependencies, and the one-line command to run and commit a fix was fairly obvious. It is my opinionated opinion that lockdown, basically requiring yet another "I'm just updating dependencies!" cycle to all of our applications, on top of what we already do for header/footer releases and close collaborators, would add yet another stack of nothing pull requests that need rubber-stamped basically every single month, promoting worse PR etiquette for minimal gain.

And yes, we actually need to migrate to a GitHub action-based deploy, instead of Travis CI, since we will be dropping that soon.

I would appreciate an after-class discussion to see what more senior engineers think, just in case I am not crazy.

I will happily approve a pull request in the interim that only updates the snapshots.

@FSVetaz
Copy link
Contributor Author

FSVetaz commented Oct 13, 2025

From what I understand, package-lock.json is only for the developers of that package and not for the consumers. So a "@fs/eslint-config-frontier-react": "^11.4.2", in the package.json here would still allow 11.5 etc in a consuming package even if the package-lock.json here only had 11.4.2.

If we were using, npm-shrinkwrap.json, then consumers may only be limited to that.

See https://docs.npmjs.com/cli/v6/configuring-npm/shrinkwrap-json. For now, though, I will make that other pr.

@skye2k2
Copy link
Collaborator

skye2k2 commented Oct 13, 2025

Right. That should be the case. Sorry. I overreacted, given the dependency update purgatory we are already living in.

@FSVetaz FSVetaz merged commit 455e94e into master Oct 13, 2025
1 check passed
@FSVetaz FSVetaz deleted the jordi/testupd branch October 13, 2025 19:47
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.

3 participants