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

Installing the helix-pipeline in the project confuses the execution #513

Closed
kptdobe opened this issue Feb 7, 2019 · 20 comments · Fixed by #571
Closed

Installing the helix-pipeline in the project confuses the execution #513

kptdobe opened this issue Feb 7, 2019 · 20 comments · Fixed by #571
Assignees
Labels
bug Something isn't working

Comments

@kptdobe
Copy link
Contributor

kptdobe commented Feb 7, 2019

If you need to use some helix-pipeline utils or APIs (like the VDOM API) in a pre.js, it is natural to install @adobe/helix-pipeline as a dependency of the project. But by doing so, hlx up and hlx deploy will use the locally installed helix-pipeline and as you update hlx-cli will quickly "confuse" the execution because an older (newer is possible too) version will be used.

To reproduce:

hlx demo --type full test
cd test
npm init
npm install @adobe/helix-pipeline

Edit the package.json and change the @adobe/helix-pipeline dependency to a previous version (simulate what will happen when you upgrade hlx-cli but forget to align the dependency here).
Run npm install and finally run hlx up. If you are lucky, it might work but there are great chances that the current helix-cli requires a newer version of the pipeline and fails.

This is pretty vicious and you might remove the dependency from the package.json but do not remove it from the node_modules folder. The helix-cli ends up in a corrupted state which is really hard to identify why.
We should protect our end-user to reach that point.

Note that this should be true if you install @adobe/helix-cli or any library that have the pipeline as a dependency.

cc @tripodsan

@kptdobe kptdobe added the bug Something isn't working label Feb 7, 2019
@tripodsan
Copy link
Contributor

I think that the locally installed modules should win over the ones provided by helix-cli, since it is what the user expects. It is important that hlx up and hlx package use the same modules.

@kptdobe
Copy link
Contributor Author

kptdobe commented Feb 8, 2019

Yes, I agree. But we need to make sure people include the pipeline only if they really know what they are doing and for that, we would need to extract the "utils" stuff (like the VDOM APIs) that people may need more frequently without the need to get the ownership of the full pipeline.

@tripodsan
Copy link
Contributor

...without the need to get the ownership of the full pipeline.

but we anyways include the entire helix-pipeline into the deployed action. so if they choose to have a different version in their local project due to backward compatibility reason, the local version should be included.

maybe, on the long run, it would even be better, to not to provide any runtime packages with helix-cli, and force people to include helix-pipeline in their projects. so it is more transparent what is included in the action. we could bundle all that's needed in a helix-ow-runtime, so they need to include less packages.

@trieloff WDYT?

@kptdobe
Copy link
Contributor Author

kptdobe commented Feb 8, 2019

That's probably where we have the biggest disagreement ;)

In the default and basic use of the product, you do not want to think about the pipeline, which version, backward comp... it should just work. And that's not the case if you add the pipeline to use the VDOM API. But we engineer this stuff the other way around, for the worst case scenario and the customer which will want to customise everything.
And even the last is not a requirement: a customer never wants to customise anything, he just reaches a point where the product does not allow what he wants to do.
I would rather lock everything down and wait for the day a customer asks us for a use case that we do not want to cover by an existing feature and a new one we would build for them. That day, they will be allowed to start messing around with the pipeline version but with the contract that they have to deal with upgrades, backward comp, maintenance...

@tripodsan
Copy link
Contributor

I would rather lock everything down

ok, what we could do, is to check if the local installed version of the pipeline is the same as the one in helix-cli. this would probably even be simple than messing with the module loader to use the correct modules.

@trieloff
Copy link
Contributor

I think breaking the pipeline-utils into a separate package/repo would be a good thing to do anyway.

This way we minimize the exposure of things that a customer would want to forcibly upgrade/downgrade.

@tripodsan
Copy link
Contributor

I think breaking the pipeline-utils into a separate package/repo would be a good thing to do anyway.

it will always be a problem when people using different versions of modules in the local development as helix uses in cli. I don't think it will help when splitting the pipeline apart to solve this problem.

@tripodsan
Copy link
Contributor

the clean approach would really be to force people to use helix-pipeline in their package. as I wrote above:

it would even be better, to not to provide any runtime packages with helix-cli, and force people to include helix-pipeline in their projects. so it is more transparent what is included in the action.

@trieloff
Copy link
Contributor

But that also means you must have a package.json, which adds some initial complexity to each project.

@tripodsan
Copy link
Contributor

... which adds some initial complexity to each project.

true. but we can generate it on hlx demo. but of course it's a clash to the simple-setup paradigm.

@trieloff
Copy link
Contributor

Going package.json-less was fun while it lasted, but sooner or later projects will have to have a package.json anyway, so that should not stop us from doing things the right way. +1 to your proposal.

@tripodsan
Copy link
Contributor

maybe we can be smart about it:

  • if there is no package.json - or if no @adobe/* package is defined, we automatically inject helix-runtime somwhow
  • if there is a package.json and at least one @adobe/* package is defined, we just use helix-cli proper and assume all dependencies needs to be provided with the project.

@trieloff
Copy link
Contributor

  • or if no @adobe/* package is defined

Let's leave out that condition and simply have a default-package.json that is being used when no package.json is there.

@tripodsan
Copy link
Contributor

Suggestion:

  • remove @adobe/helix-pipeline from helix-cli as dependency
  • hlx up, hlx build, hlx demo --type-full
    • if a package.json exists, check if @adobe/helix-pipeline is present and if not show an error to the user.
    • if no package.json is present, generate a one and execute npm add @adobe/helix-pipeline
  • hlx deploy will execute hlx buld (see [hlx deploy] must run hlx build #241)

@tripodsan tripodsan self-assigned this Feb 19, 2019
@tripodsan
Copy link
Contributor

tripodsan commented Feb 19, 2019

I was looking more deeply into this problem. so from the helix-cli context, it doesn't see the node_modules of the project:

i.e. when listing the module path during hlx up we see:

[ '/Users/nobody/codez/helix-cli/src/node_modules',
  '/Users/nobody/codez/helix-cli/node_modules',
  '/Users/nobody/codez/node_modules',
  '/Users/nobody/node_modules',
  '/Users/node_modules',
  '/node_modules' ]

in hlx-simulator, we tweak the module loader path, so that it can load the modules from helix-cli, but also, never from the current project. so AFAICT, having a helix-pipeline in the project should not have any impact on hlx up or hlx package.

but I can reproduce your problem @kptdobe. so maybe it's a problem with the helix-simulator. since it concatenates the helix-cli paths to the module paths of the templates. for example:

[ '/Users/nobody/codez/helix/sandbox/test/.hlx/build/node_modules',
  '/Users/nobody/codez/helix/sandbox/test/.hlx/node_modules',
  '/Users/nobody/codez/helix/sandbox/test/node_modules',
  '/Users/nobody/codez/helix/sandbox/node_modules',
  '/Users/nobody/codez/helix/node_modules',
  '/Users/nobody/codez/node_modules',
  '/Users/nobody/node_modules',
  '/Users/node_modules',
  '/node_modules',
  '/Users/nobody/codez/helix-cli/src/node_modules',
  '/Users/nobody/codez/helix-cli/node_modules',
  '/Users/nobody/codez/node_modules',
  '/Users/nobody/node_modules',
  '/Users/node_modules',
  '/node_modules' ]

which makes it use the helix-pipeline of the local project.

@tripodsan
Copy link
Contributor

I also verified the packager. it includes local project modules, but always takes precedence of the modules provided by helix-cli if present. this is done during the parcel-external-module analyzer (which uses the internal parcel mechanics). so it is probably difficult to change this behaviour.

so for now, it is probably easier to fix the behaviour in the simulator to use the helix-cli modules first before the project modules. changing the order seems to be difficult.

@filmaj
Copy link
Contributor

filmaj commented Feb 20, 2019

Just catching up on all the helix goodness that's happened over the past few weeks. So what is the recommendation at this point, given the discussion here? Leave the dependency out of a helix site's package.json, or go ahead and add it in? I'm trying to see if it is worth reopening and merging https://github.com/adobe/developer.adobe.com/pull/115 or not.

@tripodsan
Copy link
Contributor

@filmaj with the current version (v0.13.5), having the helix-pipeline in the local project is no longer a problem.

@filmaj
Copy link
Contributor

filmaj commented Feb 21, 2019

Only the pipeline is safe to include, or is helix-cli ok to include as well?

@tripodsan
Copy link
Contributor

Only the pipeline is safe to include, or is helix-cli ok to include as well?

should be ok..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants