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

Consider removing node.js dependency (OCC-314) #527

Open
sarahelsaig opened this issue Feb 25, 2025 · 7 comments
Open

Consider removing node.js dependency (OCC-314) #527

sarahelsaig opened this issue Feb 25, 2025 · 7 comments
Labels
enhancement New feature or request

Comments

@sarahelsaig
Copy link
Contributor

sarahelsaig commented Feb 25, 2025

The goal is to elminate a possible failure point in build (see #524) and to improve performance because node can be tragically slow, especially for Windows developers.

Transcript from internal discussion:

I've looked over the project to see where do we need NE for in OCC. Basically we do two things:

  • Lint JS files using ESLint.
  • Convert 3 very simple Sass files to CSS.
    The JS files are very simple. We don't use libraries from NPM, we don't have JS files that use import, etc. So if not for the linting we could copy them into wwwroot as-is. We'd lose minification, as if anyone cared about that. So I wonder if maybe there is a different way to lint JS? Any thoughts?

As for the Sass files, one of them we could just rename to CSS and it would work. The other two are also simple enough to be converted into CSS without losing much clarity. They use import to share an errors style sheet but that could be its own CSS, made a dependency in OC's resource management instead. Linting is a consideration here too, but due to the simplicity of the handful of stylesheets (and my disinterest in having an official default e-commerce stylesheet in OCC) we can forgo that if it's the only thing stopping us from ejecting this expensive and fragile dependency.
OrchardCMS/OrchardCore.Commerce

Possible considerations so far:

  • Lint JS and CSS separately via GitHub actions, if we can still use a shared linter configuration.

Jira issue

@sarahelsaig sarahelsaig added the enhancement New feature or request label Feb 25, 2025
@github-actions github-actions bot changed the title Consider removing node.js dependency Consider removing node.js dependency (OCC-314) Feb 25, 2025
@Piedone
Copy link
Member

Piedone commented Feb 25, 2025

Yeah, I'd also remove NE in this case, but linting should really stay, so that's not enough, yet. What NE provides for just linting are two things:

  1. Common linting config: it contains the common Lombiq config itself (part of which comes from NPM from third-party packages that the Lombiq config extends) and the setup of the linting config files (it copies an .eslintrc.js, .prettierrc.js, .stylelintrc.js to the root). This also makes it possible to see the linter violations in the IDE (what for VS is only ESLint, but other IDEs also work with stylelint).
  2. Installs the linter packages and runs them as part of MSBuild.

Even if we run linting just in CI (or from the CLI, installed locally based on instructions and not automatically), a way to do #1 is necessary. Perhaps an otherwise inert submodules, that just stores the config files, may be suitable (or does what NE does, without hooking the linting itself into MSBuild). We can skip #2.

@Skrypt
Copy link
Contributor

Skrypt commented Feb 25, 2025

Why not use the new Asset Manager instead here too?

@sarahelsaig
Copy link
Contributor Author

sarahelsaig commented Feb 25, 2025

Why not use the new Asset Manager instead here too?

You mean this, right? This still relies on node.js and in addition it adds the dependency of Yarn. So it's at best a neutral change, at worst additional dependencies.

@Skrypt
Copy link
Contributor

Skrypt commented Feb 25, 2025

Unless you don't want to have reproducible build then you can remove Node. Else, I think having reproducible builds is a requirement for .NET Foundation projects. Unless OCC is not part of .NET Foundation I'd say it would be fine to only have compiled assets in the repository.

But I'd rather have a tool that doesn't break the server side code compilation.

@Piedone
Copy link
Member

Piedone commented Feb 25, 2025

The new asset manager doesn't bring any benefits for OCC over Node.js Extensions, what it uses currently.

@Skrypt
Copy link
Contributor

Skrypt commented Feb 25, 2025

It does the same differently but sayin that it adds any benefits would be pejorative. 😄
I just named one with server side compilation. I can't actually build the Commerce module right now because of a Nuget package dependency needing a fix...

@sarahelsaig
Copy link
Contributor Author

Unless you don't want to have reproducible build then you can remove Node. Else, I think having reproducible builds is a requirement for .NET Foundation projects. Unless OCC is not part of .NET Foundation I'd say it would be fine to only have compiled assets in the repository.

But I'd rather have a tool that doesn't break the server side code compilation.

As stated above, we already don't do any code compilation for JS. And for the handful of very simple SCSS files, it's just not worth to keep them as SCSS. So the point is that we shouldn't really need server side compilation for OCC at all, broken or otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants