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

[WIP] update:ui - support node_modules locations outside of gem engine dirs #4435

Closed
wants to merge 2 commits into from
Closed

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Aug 8, 2018

this depends on a core PR (ManageIQ/manageiq#17818) to provide the right path to node_modules via AssetPath

On our side, we just use the directories provided.

(except for running webpack, using the dir even there, postcss config, and typescript)

@himdel
Copy link
Contributor Author

himdel commented Aug 8, 2018

This almost works now, except:

  • yarn --modules-folder will not create node_modules/.bin - we have to run webpack as $node_modules/webpack-cli/bin/cli.js
  • webpack will fail to load anything it itself needs (requires in our config) - we have to set NODE_PATH to the right node_modules location
  • postcss will fail to compile any css, as it looks for config in parent directories .. we have to do cp manageiq-ui-classic/.postcss.yml /tmp/test/
  • typescript is not reliably using webpack loaders, complains about not being able to find external modules (we can probably create an externals file and feed typescript from that)

So... with the two PRs this works except for the typescript bits:

cp .postcssrc.yml /tmp/test
NODE_PATH=/tmp/test/manageiq-ui-classic/node_modules NODE_ENV=development /tmp/test/manageiq-ui-classic/node_modules/webpack-cli/bin/cli.js  --config config/webpack/development.js 

EDIT: also, yarn modules-folder will still create yarn.lock in the plugin dir; we will need to copy package.json to the right test folder and cd there before running it (at which point we don't need modules-folder anymore)

this depends on a core PR (ManageIQ/manageiq#17818) to provide the right path to node_modules via AssetPath

On our side, we just use the directories provided.

(except for running webpack, using the dir even there, postcss config, and typescript)
@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2018

Some comments on commits https://github.com/himdel/manageiq-ui-classic/compare/5cb108fd2c918a4897be82fc26255215f6c95530~...d7ce729c218e2c89f322abd6f46182f320527d4d

lib/tasks/manageiq/ui_tasks.rake

  • ⚠️ - 31 - Detected puts. Remove all debugging statements.
  • ⚠️ - 32 - Detected puts. Remove all debugging statements.

@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2018

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/5cb108fd2c918a4897be82fc26255215f6c95530~...d7ce729c218e2c89f322abd6f46182f320527d4d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy
Copy link
Member

Fryguy commented Aug 10, 2018

Core PR ManageIQ/manageiq#17818 merged

@Fryguy Fryguy closed this Aug 10, 2018
@Fryguy Fryguy reopened this Aug 10, 2018
@@ -9,7 +9,7 @@ namespace :update do
asset_engines.each do |engine|
Dir.chdir engine.path do
next unless File.file? 'package.json'
system("yarn") || abort("\n== yarn failed in #{engine.path} ==")
system("yarn --modules-folder='#{engine.node_modules}'") || abort("\n== yarn failed in #{engine.path} (output: #{engine.node_modules}) ==")
Copy link
Member

@Fryguy Fryguy Aug 10, 2018

Choose a reason for hiding this comment

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

Should this be engine.node_root? EDIT: nvm, I didn't see that this was a yarn call for each plugin

@himdel
Copy link
Contributor Author

himdel commented Aug 22, 2018

Re typescript

Looks like babel6 does not support enough typescript to skip using tsc.
Looking for a way to tell tsc how to load things from elsewhere, or no to try.
If that fails, this will depend on #3784 (or we'd need to drop typescript support) (EDIT: #6211)

@miq-bot
Copy link
Member

miq-bot commented Nov 26, 2018

This pull request is not mergeable. Please rebase and repush.

@himdel
Copy link
Contributor Author

himdel commented Jan 24, 2019

Possible alternative: #5196

Yarn provides a plug&play mode where node_modules is not created inside the repo and handled by node itself; instead,yarn creates a path definition file that node can use to find the right paths.

The downside from the view of the purpose of this PR is that currently it still tries to create yarn.lock and .pnp.js locally.

The advantage is that according to https://yarnpkg.com/en/docs/pnp/getting-started, typescript is handled (though by a different loader than we're using).

@himdel
Copy link
Contributor Author

himdel commented Jun 13, 2019

Small progress - dropping typescript in #5707

@himdel
Copy link
Contributor Author

himdel commented Dec 10, 2019

tink may also become a viable alternative (a npm project, no node_modules, can lazy-download dependencies once the code actually tries to use them, but still early)

EDIT: there's also pnpm, but that does create node_modules, only with links instead of copies (so no huge dirs, but still dirs with huge amounts of files).

@Fryguy
Copy link
Member

Fryguy commented Dec 12, 2019

@himdel I see you are making progress the last couple days, but it's a little hard to follow the various PRs and repos. Can you give a tl;dr on what you've found?

@himdel
Copy link
Contributor Author

himdel commented Dec 16, 2019

@Fryguy sure...

We're special because of the way we do UI plugins - the plugins can have their own package.json dependencies, while ui-classic does not depend on any of the plugins via npm, causing us to have multiple dependency trees with no single root. That's the only problem stopping us from using yarn plug & play now (it works if I disable the plugins).

So, the idea is to, either:

  • make this into a single dependency tree:
    • either by having ui-classic list the plugins in package.json as dependencies
      • works now, but makes us download each plugin once more, via npm (when not using override_gem for a local dev copy)
    • or by using yarn workspaces to achieve pretty much the same
      • could work without extra copies, I haven't done this yet
  • drop the ability for plugins to add any npm dependencies
    • only really affects v2v

@Fryguy
Copy link
Member

Fryguy commented Jan 15, 2020

@himdel I'm interested in the yarn namespaces idea. I'd really like to avoid drop the ability for plugins to add any npm dependencies, as being able to extend the UI in a pluggable way seems like a good feature.

@himdel
Copy link
Contributor Author

himdel commented Jan 16, 2020

@Fryguy Agreed it's a good feature to support, I'll see how it goes :).
(However, as of right now, we're talking about 10 v2v dependencies, so, sometimes I wonder if the feature is worth the overhead.)

@Fryguy
Copy link
Member

Fryguy commented May 24, 2022

yarn 3 helps a lot more with this, particularly when using the global cache. When we get to Plug'n'Play it should go away. Closing for now

@Fryguy Fryguy closed this May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants