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.

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

test: use workspace package versions #261

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

nobkd
Copy link
Collaborator

@nobkd nobkd commented Apr 11, 2024

Previous code description
  1. list package dirs
  2. for each dir's package.json
    1. load json
    2. iterate over dependencies
      1. check if name starts with nue
      2. if true, remove from dependencies -> uses workspaces variant
    3. write updated json
import { readdir, readFile, writeFile } from 'node:fs/promises'

const pkgs = (await readdir('packages', { withFileTypes: true }))
    .filter(dirent => dirent.isDirectory())
    .map(dir => `packages/${dir.name}/package.json`)

for (const pkg of pkgs) {
    const json = JSON.parse(await readFile(pkg, { encoding: 'utf8' }))
    for (const key of Object.keys(json.dependencies ?? {})) {
        if (key.startsWith('nue')) delete json.dependencies[key]
    }
    await writeFile(pkg, JSON.stringify(json))
}

Maybe I have it completely wrong, and the workspace versions are already used on tests, but I'm not too sure about that.


We will have to check, if there isn't a better way. Because, this behavior should also go for local development... (but i think the preinstall script of the base workspace was not enough, because the subpackages get initialized before the preinstall script?)

What I also dislike with this approach is, that the package files are changed and accidental commit to git can happen easily...

There's also the problem with the handling of releases.. There, proper versions should be used, but when some get updated.... We'll, it sure. Lost my train of thought...

@nobkd nobkd requested a review from fritx April 11, 2024 15:15
@nobkd nobkd mentioned this pull request Apr 11, 2024
2 tasks
@fritx
Copy link
Collaborator

fritx commented Apr 15, 2024

Looks a little tricky, how do other people do for their monorepo projects?
Why doesn't "workspace" work in nuejs ? 😅

@nobkd

This comment was marked as outdated.

@nobkd nobkd force-pushed the test/workspace-versions-for-testing branch 5 times, most recently from 3a5112c to 1a6180e Compare April 15, 2024 10:31
@nobkd

This comment was marked as outdated.

@nobkd nobkd changed the title test: use workspace versions on test test: use workspace package versions Apr 15, 2024
@fritx
Copy link
Collaborator

fritx commented Apr 16, 2024

It's unlucky that npm doesn't support such syntax and standards if it is true.

I personally recommend CI to cover npm install, because npm is always the most basic compatibility object, although in many cases pnpm can bring significant optimization effects.

But if this update can solve the headaches you guys have been experiencing these days, I think we can just move forward. Cheers! 🍻

@tipiirai
Copy link
Contributor

Does this mean that npm no longer works after merging this PR?

@nobkd

This comment was marked as outdated.

@nobkd nobkd marked this pull request as draft April 16, 2024 08:11
@nobkd
Copy link
Collaborator Author

nobkd commented Apr 16, 2024

Important

bun and npm use the workspace package version when I use * instead of latest...
But this is still not great for releases, because the latest cached version could theoretically be used of some package (with *) instead of the real latest one.

Best would probably be real version specifiers like e.g. ^X.X.X, but I think there was some reason that latest was used...
@tipiirai do you remember why this was needed? Just for convenience? See 1c2e6e9

@nobkd nobkd force-pushed the test/workspace-versions-for-testing branch from 44f2aec to 2551d95 Compare April 16, 2024 12:40
@nobkd nobkd requested a review from tipiirai April 16, 2024 12:57
@nobkd nobkd marked this pull request as ready for review April 16, 2024 13:05
@tipiirai
Copy link
Contributor

I started using latest after seeing issues from forgetting to update the latest version numbers from the package.json files. I guess "*" has the same effect than latest so this looks great to me.

tried it again, and it didn't work again

So not safe to merge yet?

@fritx
Copy link
Collaborator

fritx commented Apr 17, 2024

I suddenly remembered how I handled such workspace case many years ago.
https://github.com/search?q=user%3Afritx+%22file%3Apackages%22&ref=opensearch&type=code

It works before, but I'm not sure if it will help with our current situation..

// package.json
// "dependencies": {
"react-drag-sizing": "file:packages/react-drag-sizing",
"jsdom-innertext": "file:packages/jsdom-innertext",
"jsdom-selection": "file:packages/jsdom-selection",

UPDATE:

Here npm install linking is working again:

cd nuejs/nue
rm -rf node_modules
npm install
ls -l node_modules | grep nue
>> 
lrwxr-xr-x   1 fritx  staff   16  4 17 12:33 nue-glow -> ../packages/glow
lrwxr-xr-x   1 fritx  staff   17  4 17 12:33 nuejs-core -> ../packages/nuejs
lrwxr-xr-x   1 fritx  staff   18  4 17 12:33 nuekit -> ../packages/nuekit
lrwxr-xr-x   1 fritx  staff   19  4 17 12:33 nuemark -> ../packages/nuemark

ls -l packages/nuekit/node_modules | grep nue
>> 
lrwxr-xr-x  1 fritx  staff    14B  4 17 12:33 nue-glow -> ../../nue-glow
lrwxr-xr-x  1 fritx  staff    14B  4 17 12:33 nuemark -> ../../nue-mark

But I'm not sure if the dependency version is gonna be replaced properly when npm publish...

@nobkd
Copy link
Collaborator Author

nobkd commented Apr 17, 2024

tried it again, and it didn't work again

@tipiirai that was only, because I (tried * as version and) forgot to remove the package-lock.json after I checked if linking worked with latest as version (which wasn't the case).
So the current implementation does work properly and is ready to be merged, but I'm not sure, if it is that great, because cached versions could also be used with * instead of real latest... 🤷

@fritx The file:packages/<dir> sounds interesting and works with npm, but sadly does not with bun. And there would also be the issue with possible need to replace on publish.

In conclusion, I think, the * version is the best thing we can get without having to change the version for publishing or using a different package manager (I have tested * with npm, pnpm and bun).

Edit: I'm done now

@nobkd nobkd force-pushed the test/workspace-versions-for-testing branch 2 times, most recently from fe18a72 to 12597a0 Compare April 17, 2024 09:26
@nobkd nobkd force-pushed the test/workspace-versions-for-testing branch from 12597a0 to f0c7a91 Compare April 17, 2024 09:30
@tipiirai tipiirai merged commit 13a7bf8 into nuejs:master Apr 22, 2024
3 checks passed
@tipiirai
Copy link
Contributor

Makes sense. Thank you!

@nobkd nobkd deleted the test/workspace-versions-for-testing branch April 22, 2024 07:37
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