Skip to content

Conversation

@ChristianMurphy
Copy link
Member

Checklist
Description of change

https://docs.npmjs.com/cli/v7/using-npm/workspaces

locking for all packages within the project is now handled by the root
lockfile

resolves #3262 and #3265

https://docs.npmjs.com/cli/v7/using-npm/workspaces

locking for all packages within the project is now handled by the root
lockfile
@ChristianMurphy
Copy link
Member Author

used --force mode to override warnings about peer dependencies for now

Comment on lines 391 to +394
- name: Run tests
working-directory: oeq-ts-rest-api
run: |
npm cit
npm t
Copy link
Member Author

Choose a reason for hiding this comment

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

this could be changed to

      - name: Run tests
        run: |
          npm t --workspace=oeq-ts-rest-api

if preferred

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer leave as it is.

@ChristianMurphy ChristianMurphy requested a review from a team August 11, 2021 04:07
@ChristianMurphy ChristianMurphy added dependencies Pull requests that update a dependency file enhancement help wanted labels Aug 11, 2021
@ChristianMurphy
Copy link
Member Author

If this is a direction the team wants to go, code build may need to be updated to run install once at the top level.

Copy link
Contributor

@PenghaiZhang PenghaiZhang left a comment

Choose a reason for hiding this comment

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

Thanks @ChristianMurphy !!! I like this idea.

@PenghaiZhang
Copy link
Contributor

PenghaiZhang commented Aug 11, 2021

Have some errors complaining error Error: Cannot parse file package.json

@ChristianMurphy
Copy link
Member Author

Have some errors complaining error Error: Cannot parse file package.json

Resolved in 8693b4f

Now there seems to be an issue with RunTypes, which looks to be the same one see in #3247

@PenghaiZhang
Copy link
Contributor

Have some errors complaining error Error: Cannot parse file package.json

Resolved in 8693b4f

Now there seems to be an issue with RunTypes, which looks to be the same one see in #3247

Do you mean the Storybook dependency conflicts ?

@ChristianMurphy
Copy link
Member Author

Do you mean the Storybook dependency conflicts ?

I think this is RunType related: https://github.com/openequella/openEQUELLA/pull/3275/checks?check_run_id=3305552177#step:10:72 🤔
It looks a lot like the error from #3247 (https://github.com/openequella/openEQUELLA/pull/3247/checks?check_run_id=3224768826#step:12:70)

@ChristianMurphy
Copy link
Member Author

The error in the storybook build https://github.com/openequella/openEQUELLA/pull/3275/checks?check_run_id=3305552249#step:6:72
looks to be related to #3275 (comment)

"prepare": "husky install"

is still running even though the root install is not run, will have to see if there's a way to skip the script

@edalex-ian
Copy link
Member

Thanks for taking this for a spin @ChristianMurphy 🙇

Long term we still hope to break out the REST Module (oeq-ts-rest-api) and possibly even the Front End (react-front-end) and so I'm trying to understand if this kind of move will make that harder. Thoughts?

Also, some of these are completely independent - such as autotest/IntegTester/ps and Source/Plugins/Core/com.equella.core/test/javascript (and kind of Source/Plugins/Core/com.equella.core/swaggerui) - so do we really want them all linked together?

Lastly, although maybe just a personal perception, doesn't it seems odd to manage this with workspaces and in a monorepo kind of way when first and foremost it's really a Java/Scala project being built via SBT. Could this confuse things? (That said, due to our root package.json this is already a bit blurred.)

I wonder if we're at the point where we do need to push out at least the REST Module. It seems that'd significantly solve the issues we're having. 🤔

(And as for runtypes, gah! I wonder why that is now raising it's head - something else must be slightly odd with the setup....)

@ChristianMurphy
Copy link
Member Author

Long term we still hope to break out the REST Module (oeq-ts-rest-api) and possibly even the Front End (react-front-end) and so I'm trying to understand if this kind of move will make that harder. Thoughts?

I think not. It mainly impacts package-lock.json, the packages and scripts themselves are mostly unchanged.
It may even simplify the process, since linked pacakge (which workspaces enables) behave more like packages pulled from npm itself.

Also, some of these are completely independent - such as autotest/IntegTester/ps and Source/Plugins/Core/com.equella.core/test/javascript (and kind of Source/Plugins/Core/com.equella.core/swaggerui) - so do we really want them all linked together?

That is a really good question, which I don't have a good answer for.
I don't think it will cause any harm, lockfile version 2 still allows them to version packages independently, and uses mostly the same folder layout as non-workspace projects would have.

Lastly, although maybe just a personal perception, doesn't it seems odd to manage this with workspaces and in a monorepo kind of way when first and foremost it's really a Java/Scala project being built via SBT. Could this confuse things? (That said, due to our root package.json this is already a bit blurred.)

Maybe?
I don't see oEQ is a project so much as a federation of projects, with loosely tied together build tools.

Wanting them managed by a single build tool is a perfectly fine goal.
I don't think that npm workspaces would stand in the way of that goal.
If SBT will become the one tool to rule them all, it would mostly hinge on SBT having plugins/integrations to manage other build tools.

uPortal is a similarly eclectic project, which also went for unifying under one build tool.
There Gradle is the parent build system, it manages several Gradle sub-projects, several of those sub-projects use https://github.com/node-gradle/gradle-node-plugin to managed node builds (gradle node plugin downloads it's own node and npm on demand and manages npm build steps along side java ones)
With everything under the Gradle unbrella, ./gradle install is the only step needed to setup and build the project.

Maybe SBT has similar integrations, that could be pulled in? or similar integrations could be built out for oEQ?
As an alternative gradle has a lot more integrations pre-built and from my experience is significantly faster than SBT.

@edalex-ian
Copy link
Member

Heya @ChristianMurphy

Thank you for your considered response as always.

I think at the end of the day we need to do something here as we're starting to have all manner of things going pear shaped in CI land since upgrading Node/NPM. Yesterday we had a number of odd NPM related build failures on GHA for renovate updates.

I like your assessment that in theory moving to workspaces shouldn't make it any harder to split later, so for now maybe it is the easiest path forward. (And hey, it'll also kind of provide us a catalogue of all the nested JS projects.)

And yes, I still dream and hope for the day when time permits a migration to gradle. Although we do have some custom tasks for SBT to do some of our JS building, it's so slow and heavy that we obviously still run a lot of things independently rather than having any smooth unification of build tooling. (And you definitely wouldn't do any commit hooks reliant on SBT launching first.) One day....

I see we've caused a number of conflicts here - due to package-lock-json updates so should be easy to address. But if we can see this succeed with a green build, then yes, let's proceed and take it for a spin.

"react-router-hash-link": "2.4.3",
"react-visibility-sensor": "3.14.0",
"runtypes": "6.3.1",
"runtypes": "6.3.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

runtypes issues appear to have come from a duplicated version.
oeq-ts-rest-api uses a version range

"runtypes": "^6.0.0",

this doesn't.
When those get out of sync RunTypes errors start popping up.
Long term, the version range (or lack of version range) should be synced.
Short term, I'm locking them to the same version.


Compile / resourceGenerators += Def.task {
val baseSwagger = baseDirectory.value / "swaggerui"
Common.nodeInstall(baseSwagger)
Copy link
Member Author

Choose a reason for hiding this comment

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

alternatively this could cd to the top level and run the install again there.

Comment on lines +22 to +26
- (npm ci)
- (cd Source/Plugins/Kaltura/com.tle.web.wizard.controls.kaltura/js && npm run build)
- (npm run check:ts && npm run check:ts-types-source && npm run check:license)
- (cd react-front-end && npm test)
- (cd Source/Plugins/Core/com.equella.core/test/javascript && npm cit)
- (cd Source/Plugins/Core/com.equella.core/test/javascript && npm test)
Copy link
Member Author

Choose a reason for hiding this comment

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

this is my best guess at the changes, I can't see the CodeBuild logs to confirm

add todo for running install once at top level
Comment on lines +237 to +238
// TODO: call once at top level to install all dependencies
// Common.nodeInstall(dir)
Copy link
Member Author

Choose a reason for hiding this comment

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

long term, calling this once at the top level of SBT or Gradle could give the more unified feel.
npm would still be there, but be managed by the Java/Scala build tool.

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

Labels

dependencies Pull requests that update a dependency file enhancement help wanted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants