Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump dateformat from 3.0.3 to 4.5.1. Closes #1324. #1326

Merged
merged 1 commit into from
Feb 19, 2024
Merged

Bump dateformat from 3.0.3 to 4.5.1. Closes #1324. #1326

merged 1 commit into from
Feb 19, 2024

Conversation

rcyeh
Copy link
Contributor

@rcyeh rcyeh commented Jan 17, 2024

Bump dateformat from 3.0.3 to 5.0.3. Closes #1324

@riccardoferretti
Copy link
Collaborator

There seems to be an issue with the import

@rcyeh
Copy link
Contributor Author

rcyeh commented Jan 17, 2024 •
edited
Loading

I'm sorry! I will try to work on that Friday night.

@rcyeh
Copy link
Contributor Author

rcyeh commented Jan 26, 2024 via email

@riccardoferretti
Copy link
Collaborator

Hi Richard, the first thing I would advice is checking if you can run the tests in your environment before any changes (basically against the master branch).
If those tests pass, then see if they pass with the changes.

Given that the error is also happening in CI I believe the issue is not strictly due to your environment.

Another avenue could be to make sure the node version required by the library. We use node 18 (looking at the engine configuration I have a feeling we should be more restrictive with the version)
To manage node versions I would suggest looking into nvm.

@rcyeh
Copy link
Contributor Author

rcyeh commented Jan 27, 2024 via email

@rcyeh rcyeh force-pushed the issue-1324-bump-dateformat-to-503 branch 2 times, most recently from 3e79325 to 2b65c17 Compare January 27, 2024 06:58
@rcyeh
Copy link
Contributor Author

rcyeh commented Jan 27, 2024

I synced with origin/master and just took the yarn.lock from the repository. Tests continue to pass.

@rcyeh rcyeh force-pushed the issue-1324-bump-dateformat-to-503 branch 3 times, most recently from 084147a to 4eb89a9 Compare January 27, 2024 07:10
@rcyeh
Copy link
Contributor Author

rcyeh commented Jan 27, 2024

OK finally! Cleaned up the commit. To explain the added dependencies: husky is needed to run the git commit hooks. jest, jest-environment-node, and jest-environment-jsdom are needed to run tests.

@rcyeh
Copy link
Contributor Author

rcyeh commented Feb 2, 2024

Hi Riccardo, thank you for helping me get my environment working. I updated the pull request last week. Tests pass.

I was able to use node 20.11 as well.

riccardoferretti
riccardoferretti reviewed Feb 4, 2024
View reviewed changes
package.json Outdated
@@ -39,5 +39,10 @@
"singleQuote": true,
"trailingComma": "es5"
},
"dependencies": {}
"dependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these should be devDependencies, but more importantly I don't understand why they are needed. What happens if you remove them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to rename to devDependencies.

It seems that this package.json and the yarn.lock file are used to identify dependencies to be pulled in. My workflow is: rm -rf node_modules; yarn install; yarn build; yarn test.

Without "jest": "^29.7.0" I get an error:

There was an error while running the Foam suite Error: ● Validation Error:

  Preset ts-jest not found.

  Configuration Documentation:
  https://jestjs.io/docs/configuration

and the tests do not run. I've moved this to devDependencies in the latest version of this pull request.

Previously, I would get different errors without the others, but now they do not produce errors.

Because it does not seem to be necessary to assume them, I've removed the others.

@rcyeh rcyeh force-pushed the issue-1324-bump-dateformat-to-503 branch from 4eb89a9 to 45f0689 Compare February 13, 2024 07:07
@rcyeh rcyeh changed the title Bump dateformat from 3.0.3 to 5.0.3. Closes #1324. Bump dateformat from 3.0.3 to 4.5.1. Closes #1324. Feb 13, 2024
@riccardoferretti
Copy link
Collaborator

This looks good. Quick q: is there a reason for a upgrading jest?

@rcyeh
Copy link
Contributor Author

rcyeh commented Feb 13, 2024 via email

@riccardoferretti
Copy link
Collaborator

The jest dependency

"jest": "^29.6.2",

is defined in the package.json of foam-vscode.

I wonder if things would work even if you removed the new dependency you introduced. And if that's not the case, I would update the dependency in the package, where the old one is defined.

@rcyeh
Copy link
Contributor Author

rcyeh commented Feb 13, 2024 via email

@riccardoferretti
Copy link
Collaborator

I mostly do it from the top level, but either should work

@rcyeh rcyeh force-pushed the issue-1324-bump-dateformat-to-503 branch from 45f0689 to cd97eff Compare February 14, 2024 04:06
@rcyeh
Copy link
Contributor Author

rcyeh commented Feb 14, 2024 via email

@riccardoferretti
Copy link
Collaborator

Thanks for the explanation, I did some local testing and was able to reproduce the problem.
I also managed to fix it locally by updating in foam-vscode/package.json:

  • jest to version ^29.7.0
  • @types/jest to version ^29.5.12

(and removed jest from the top level package.json)

Can you validate this set up work for you as well? Happy to then merge the PR

@rcyeh rcyeh force-pushed the issue-1324-bump-dateformat-to-503 branch from cd97eff to d9f6659 Compare February 14, 2024 12:46
@rcyeh
Copy link
Contributor Author

rcyeh commented Feb 14, 2024 via email •
edited
Loading

@riccardoferretti
Copy link
Collaborator

Mmmm.. can you try to do a yarn clean as well before the build?
Odd what's happening.

I am not strongly opposed to moving the jest dependency at the top, but in that case I would remove it from the package (or at least have matching versions)

@rcyeh rcyeh force-pushed the issue-1324-bump-dateformat-to-503 branch from d9f6659 to 5850070 Compare February 17, 2024 22:03
@rcyeh
Copy link
Contributor Author

rcyeh commented Feb 17, 2024 •
edited
Loading

Thank you! yarn clean was what I needed! With the dateformat dependency update, there was a corresponding small change in yarn.lock, so I added that to the change. The tests all work now, without any change to the top-level (or any other) package.json.

I tried to delete the dateformat@^3.0.0 section from yarn.lock; but yarn install put it back.

riccardoferretti
riccardoferretti approved these changes Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks :) will merge after the test run

@riccardoferretti riccardoferretti merged commit 9a027c0 into foambubble:master Feb 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riccardoferretti riccardoferretti riccardoferretti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update dateformat requirement to latest (5.0.3)
2 participants
@rcyeh @riccardoferretti