fix: await Environment.lookup (#132)

SAP / generator-easy-ui5

Meta-generator various project types within the UI5 Universe

https://blogs.sap.com/2021/04/09/easy-ui5-3.0-from-community-contributions-to-community-plugins/

Apache License 2.0

230 stars 72 forks source link

fix: await Environment.lookup (#132) #133

Closed cpa-gecko closed 7 months ago

cpa-gecko commented 7 months ago

Environment.lookup seems to return a promise now, i suggest to await the result to fix the error in #132

cla-assistant[bot] commented 7 months ago

All committers have signed the CLA.

vobu commented 7 months ago

I've made some minor changes in my local copy of the PR... @cpa-gecko, could you please grant me rw on your fork so I can push to it? Thanks! Once the updates are in, good to test this PR, thus pinging @fleischr here as well :)

vobu commented 7 months ago

To add some more context: the issue of the async env.lookup() stems from yeoman-environment^4. In ^3 it's still a sync function.

Yet I could not reproduce the bug from the issues described in #132

both node 18 + 20 worked flawlessly out of the box for me with a vanilla yo easy-ui5 project and yo easy-ui5 --plugins
I searched through the UI5-related (sub-)generators for yeoman-environment^4, but all are on ^3

So my guesstimate is that the people affected by #132 must have gotten the module dependency yeoman-environment^4 through some other channel.

Anyway, adding an async to env.lookup will IMO do no harm as it

will fix #132 when yeoman-environment is at ^4
will only advance the node event loop for yeoman-environment^3

vobu commented 7 months ago

ready for review @fleischr - and for your approval as well @petermuessig ;)

petermuessig commented 7 months ago

As this is compatible also with older version of yeoman-environment I'm fine with it and will immediately release a patch for the fix

petermuessig commented 7 months ago

@cpa-gecko @fleischr @vobu - thanks for following-up on this!

petermuessig commented 7 months ago

@cpa-gecko @fleischr @vobu - one thing which makes me a bit wondering is the min requirement for Node 18.17.0 - many people may have updated but some may have older versions. This comes with the dependency to yeoman-environemnt 4.13.0

Just checked, BAS is using 18.14.2 currently and the usage may not be possible anymore. I'm checking the deps and how to allow at least older minors of 18.

vobu commented 7 months ago

Node 16 is out of maintenance, substituted by Node 18. Node 20 is the new LTS. https://nodejs.org/en/about/previous-releases That's why +1 from me for >= 18. (GH Actions now run with Node 20 already)

petermuessig commented 7 months ago

It's now >= 18 and not >=18.17 which is OK IMO

https://www.npmjs.com/package/generator-easy-ui5 => 3.7.0 is out

cpa-gecko commented 7 months ago

fyi - yeoman-environment is installed with (e.g. on my machine - global) yo v5.0.0

see: https://github.com/yeoman/yo/blob/c335710bb74744b2712e4fd80c1190f2650f3b83/package.json#L73