embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
330 stars 137 forks source link

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

Open NullVoxPopuli opened 2 years ago

NullVoxPopuli commented 2 years ago

related to: https://github.com/embroider-build/embroider/issues/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 commented 2 years ago

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 commented 2 years ago

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

NullVoxPopuli commented 2 years ago

nevermind this is resolved with 0.50.0

NullVoxPopuli commented 2 years ago

j/k, I still had my patch script running

NullVoxPopuli commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

NullVoxPopuli commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 https://github.com/emberjs/ember-render-modifiers/pull/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 commented 2 years ago

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 commented 2 years ago

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

NullVoxPopuli commented 2 years ago

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

Jopie01 commented 2 years ago

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 commented 2 years ago

I can reproduce this myself now, but it's only because we've released https://github.com/embroider-build/embroider/pull/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 https://github.com/emberjs/ember-render-modifiers/pull/58

Jopie01 commented 2 years ago

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 commented 2 years ago

Issue remains for me: image

Jopie01 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

NullVoxPopuli commented 2 years ago

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

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

https://github.com/mixonic/ember-cli-deprecation-workflow/issues/133

NullVoxPopuli commented 2 years ago

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 commented 2 years ago

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 :thinking:

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 :upside_down_face: )

NullVoxPopuli commented 2 years ago

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 commented 2 years ago

so, includePrelease only affects the range argument of satisfies :thinking:

NullVoxPopuli commented 2 years ago

I think I know how to resolve this. PR incoming