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

@embroider/macros' dependencySatisfies does not understand canary versions #1066

Open
NullVoxPopuli opened this issue Jan 7, 2022 · 28 comments

Comments

@NullVoxPopuli
Copy link
Collaborator

related to: #1057

version is: DEBUG: Ember : 4.2.0-alpha.1.canary+ef2ad15f

my hunch is that the existing comparison code isn't expecting this many specifiers after alpha.1

@ef4
Copy link
Contributor

ef4 commented Jan 7, 2022

Please be more specific.

We use semver.satisfies with the includePrerelease: true option, which definitely does understand versions like that:

require('semver').satisfies('4.2.0-alpha.1.canary+ef2ad15f', '>=4.0', { includePrerelease: true })
// true

@NullVoxPopuli
Copy link
Collaborator Author

I have the same issue with the latest beta. I'll spelunk soon

@NullVoxPopuli
Copy link
Collaborator Author

nevermind this is resolved with 0.50.0

@NullVoxPopuli
Copy link
Collaborator Author

j/k, I still had my patch script running

@NullVoxPopuli NullVoxPopuli reopened this Jan 9, 2022
@NullVoxPopuli
Copy link
Collaborator Author

I quick threw a (few) console.log in the node_modules/..../dependency-satisfies.js
(and this time I remember to have rm -r /tmp/embroider/ && yarn start as my run command) 🙃

    try {
        let us = packageCache.ownerOfFile(sourceFileName);
        console.log({ us: Boolean(us), name: packageName.value });
        if (!us) {
            return false;
        }
        let version = packageCache.resolve(packageName.value, us).version;
        console.log({ name: packageName.value, version: Boolean(version) });
        return (0, semver_1.satisfies)(version, range.value, {
            includePrerelease: true,
        });
    }
    catch (err) {
        console.log('catch', { name: packageName.value, range: range.value, error: err.code, sourceFileName });
        if (err.code !== 'MODULE_NOT_FOUND') {
            throw err;
        }
        return false;
    }

It seems the host ember-source version is ignored in all these catch scenarios? I'm still not clear on how dependencySatisfies is supposed to work

catch {
  name: 'ember-source',
  range: '>= 3.22.0-beta.1',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@ember/render-modifiers/modifiers/did-insert.js'
}
{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>= 3.22.0-beta.1',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@ember/render-modifiers/modifiers/did-insert.js'
}
{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>= 3.22.0-beta.1',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@ember/render-modifiers/modifiers/will-destroy.js'
}
{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>= 3.22.0-beta.1',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@ember/render-modifiers/modifiers/will-destroy.js'
}
⠹ building... [@embroider/webpack]{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>=3.27.0-canary || >=3.27.0-beta',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@embroider/util/ember-private-api.js'
}
{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>=3.27.0-canary || >=3.27.0-beta',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@embroider/util/ember-private-api.js'
}
{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>=3.24.0-canary || >=3.24.0-beta',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@embroider/util/ember-private-api.js'
}
{ us: true, name: 'ember-source' }
catch {
  name: 'ember-source',
  range: '>=3.24.0-canary || >=3.24.0-beta',
  error: 'MODULE_NOT_FOUND',
  sourceFileName: '/tmp/embroider/19ab55/node_modules/@embroider/util/ember-private-api.js'
}

@ef4
Copy link
Contributor

ef4 commented Jan 10, 2022

If you try to manually resolve ember-source from your addon's directory, does it work?

You may need to declare a peerdependency on ember-source to guarantee that the addon will be able to resolve it.

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jan 10, 2022

this is an app -- but that kinda makes sense -- I'll have to PR to @ember/render-modifiers, and @embroider/util -- perhaps?

I just tried adding the peerDep locally (in node_modules), and that seems to work -- at the very least, I don't get into the catch block.

Building into /tmp/embroider/e7c5ca
⠙ building... [@embroider/webpack]{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
⠸ building... [@embroider/webpack]{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }
{ us: true, name: 'ember-source' }
{ name: 'ember-source', version: false }

Doesn't resolve my surface issue though:

Error while processing route: index window.Ember is undefined ../node_modules/@embroider/util/ember-private-api.js@http://localhost:4200/assets/chunk.7f6b0829a7e0623822e5.js:32454:3

It appears to me that the packageCache.reslove call is missing the version? O.o

@NullVoxPopuli
Copy link
Collaborator Author

looks like somehow yarn is installing an older version of @embroider/util :(

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jan 10, 2022

idk what craziness my app's addons are causing, but I've had to add these resolutions in my root package.json:

    "@embroider/macros": "0.50.0",
    "@embroider/core": "0.50.0",
    "@embroider/util": "0.50.0"

my surface issue remains though -- so now I need to figure out why version is undefined -- @ember/render-modifiers lack of peerDependencies seems to not matter (now that I'm using the correct version of @embroider/util, all ember-source resolves are found, but have undefined version)

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jan 10, 2022

so, for reasons unknown to me, I can still only debug with console.log (so painful, idk how people do it), and I added a log of packagePath in shared-internals/package-cache.js:


update on the above statement:

my VSCode config had a custom terminal profile. reverting to default allows javascript debugging terminal to work and auto-attach. yay!


I now see output like this:

{
  packageName: 'ember-source',
  packagePath: '/tmp/embroider/e7c5ca/node_modules/ember-source/package.json'
}
{ name: 'ember-source', version: undefined }
{ name: 'ember-source', version: undefined }

and when I cat that package.json

❯ cat /tmp/embroider/e7c5ca/node_modules/ember-source/package.json
{"name":"ember-source"}

there is no version!
(and the ember-source location is in tmp, which feels goofy to me, because, for example:

{
  packageName: 'focus-trap',
  packagePath: '/home/psego/Development/NullVoxPopuli/limber/node_modules/focus-trap/package.json'
}

but maybe the tmp location is for all packages that need a compat-adapter.

anywho, I catd the @glimmer/tracking package, it the package.json there makes way more sense:

❯ cat /tmp/embroider/e7c5ca/node_modules/@glimmer/tracking/package.json
{
  "name": "@glimmer/tracking",
  "version": "1.0.4",
   + a whole bunch of stuff -- looks like the real thing

@Jopie01
Copy link

Jopie01 commented Jan 13, 2022

I walked into a same kind of problem today. I created a new Ember app (version 4.1.0) and installed an addon which has the latest ember-render-modifier as a requirement. Building the app was no problem, but opening the app I got an error

Uncaught (in promise) Error: Invalid modifier manager compatibility specified

I was able to trace it down to the line where the check is done which capability to use.
https://github.com/emberjs/ember-render-modifiers/blob/v2.0.2/addon/modifiers/did-insert.js#L51
Unfortunately the dependencySatisfies returns false even 4.1.0 is greater then 3.22.0-beta.1. Removing the '-beta.1' didn't work either.

@ef4
Copy link
Contributor

ef4 commented Jan 13, 2022

@Jopie01 I see that ember-render-modifiers doesn't have a peerDependency on ember-source, which means npm or yarn is free to place ember-source somewhere that ember-render-modifiers can't see it. I just submitted emberjs/ember-render-modifiers#58 for that.

I don't think this is a problem with prereleases specifically, I think the dependency isn't being found at all. You can check in the terminal by changing into the directory where ember-render-modifiers is located within node_modules, starting node, and running require('ember-source/package.json').version

@Jopie01
Copy link

Jopie01 commented Jan 13, 2022

Thanks @ef4 for the answer and the PR!

I also tried your suggestion:

... changing into the directory where ember-render-modifiers is located within node_modules

cd node_modules/@ember/render-modifiers

starting node, and running require('ember-source/package.json').version

This gives me 4.1.0 as output.

@ef4
Copy link
Contributor

ef4 commented Jan 13, 2022

Huh. So then my guess was wrong and something else is really going on here.

@NullVoxPopuli
Copy link
Collaborator Author

seems @Jopie01 is running in to what I've been running in to with my project! yay! I'm not crazy!

@Jopie01
Copy link

Jopie01 commented Jan 13, 2022

No idea if this is giving any information ......

I just added @embroider/macros to my app to try and see what the dependencySatisfies have to say.

In the package.json I added "@embroider/macros": "^0.50.0" and in my application route I added:

import Route from '@ember/routing/route';
import { dependencySatisfies } from '@embroider/macros';

export default class ApplicationRoute extends Route {
  init() {
    super.init(...arguments);
    console.log(dependencySatisfies('ember-source', '>=4.2.0-alpha.1.canary+ef2ad15f') );
  }
}

And it gives false as answer, which is true because I'm on 4.1.0. Changing the 4.2.0 to 4.1.0 it gives true as an answer. So it seems that the versions check is working correctly.

It also got me thinking and I added console.log(dependencySatisfies('ember-source', '>= 3.22.0-beta.1')) to the did-insert.js in the ember-render-modifier and this returned false.

@ef4
Copy link
Contributor

ef4 commented Jan 13, 2022

I can reproduce this myself now, but it's only because we've released #1070

If I forcefully downgrade @embroider/macros, it uses the old looser behavior and the check passes.

So I don't see a bug here other than emberjs/ember-render-modifiers#58

@Jopie01
Copy link

Jopie01 commented Jan 13, 2022

So I don't see a bug here other than emberjs/ember-render-modifiers#58

I yarn upgraded ember-render-modifiers with your PR and indeed it fixes this problem.

@NullVoxPopuli
Copy link
Collaborator Author

Issue remains for me:
image

@Jopie01
Copy link

Jopie01 commented Jan 13, 2022

I'm on 4.1.0. No idea if that matters. I'm doing only basic stuff what a kid can do, so no complexity of an experienced developer (I'm more a Python guy). I also don't have resolutions in my packages.json and I have only the render-modifier inside the dependencies.
Is it possible that is has something to do with caching?

But now you are on your own again 🥲

@ef4
Copy link
Contributor

ef4 commented Jan 13, 2022

@NullVoxPopuli you're talking about a different addon.

My PR to @ember/render-modifiers isn't going to fix a problem with a macro in @embroider/util.

@NullVoxPopuli
Copy link
Collaborator Author

🤔 util already has ember-source as a peer dep tho, the dependencySatisfies condition is still false

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jan 14, 2022

I see other stack trace to ensureSafeComponent, and ember-headlessui doesn't have a passing test suite with embroider:
GavinJoyce/ember-headlessui#93
because ember-cli-deprecation-workflow doesn't really work with embroider.

@ef4 how's ember-cli-deprecation-wokflow going to work?

ember-cli/ember-cli-deprecation-workflow#133

@NullVoxPopuli
Copy link
Collaborator Author

tried looking at this again, and this I'm not sure how to resolve:

image

so, the place where my dependencySatisfies check is failing is coming from ensure-safe-component, which is actually here: https://github.com/embroider-build/embroider/blob/master/packages/util/app/helpers/ensure-safe-component.js

but, @embroider/util has the peer dep: https://github.com/embroider-build/embroider/blob/master/packages/util/package.json#L36

does something along the dependency path between util and my app (headless ui, perhaps?) matter? or should it be just @embroider/util that matters?

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jan 16, 2022

Changing my ember-source version to 4.1.0 seems to get me passed the error. so, it is something with me using canary/beta versions! sus 🤔

After I change my ember-source to 4.1.0, I get this error now:
image
which was fixed in in 4.2.0-beta+ (by me 🙃 )

@NullVoxPopuli
Copy link
Collaborator Author

ah ha!
so, this is the dependencySatisfies check:

    return satisfies(pkg.version, range, {
      includePrerelease: true,
    });

and here is the a vanilla reproduction: https://runkit.com/nullvoxpopuli/semver

   satisfies('4.3.1-alpha.1', '>=3.27.0-canary || >=3.27.0-beta', {
        includePrelease: true
    })

returns false

@NullVoxPopuli
Copy link
Collaborator Author

so, includePrelease only affects the range argument of satisfies 🤔

@NullVoxPopuli
Copy link
Collaborator Author

I think I know how to resolve this. PR incoming

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

No branches or pull requests

3 participants