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

Support a project having prerelease versions #1080

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Jan 16, 2022

This is what was going on with #1066

Demonstration of semver behavior here: https://runkit.com/nullvoxpopuli/semver
Reproduction of behavior: NullVoxPopuli/limber#375
(This PR is the last thing between me and finally merging that NullVoxPopuli/limber#375 🙃 )

tldr: satisfies does not return true if the version is a pre-release. the pre-release option to satisfies is only for the range parameter

I've also been wondering if there is a better way to share code between the glimmer and babel versions of dependencySatisfies -- though, I don't know why the glimmer version exists, because I thought it was all build-time transforms anyway?

@NullVoxPopuli
Copy link
Collaborator Author

@rwjblue I removed the test with * as the first-arg to satisfies, and I found an existing test that does check for * on the range arg (the second arg) -- so * should be covered

@NullVoxPopuli NullVoxPopuli changed the title Support a project having star and prerelease versions Support a project having prerelease versions Jan 20, 2022
Copy link
Collaborator

@lifeart lifeart left a comment

Choose a reason for hiding this comment

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

@NullVoxPopuli could we have test case for “ pkg.version === undefined” ?

@NullVoxPopuli
Copy link
Collaborator Author

@lifeart, iirc, I had a really hard time reproducing that in tests -- only came upon that scenario when linking to a real app

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Jan 23, 2022

@ef4 / @rwjblue I re-added the * test, because It's a valid scenario (regardless of the validity of the version) -- common in yarn workspaces to signify "this is the latest / linked version / the code that's in the monorpeo"

(but is a separate use case from my original intent: to get ember canary working at all 🙃 )

@ef4
Copy link
Contributor

ef4 commented Jan 24, 2022

Seconding what @rwjblue said.

project.addDependency() may be misleading you because it uses the second argument for two different things. One is a version and one is a range.

On new enough versions of fixturify-project (I don't recall if that's what we're using here), there's also an explicit requestedRange option to control the range separately from the version:

https://github.com/stefanpenner/node-fixturify-project/blob/e75e373aae4086406a088c8032cb203b46c6a28d/index.ts#L113

@NullVoxPopuli
Copy link
Collaborator Author

What are remaining tasks here?
Should we talk about this on a call?
(I'm trying not to forget about this during the weekly Discord events) 🙃

@lifeart
Copy link
Collaborator

lifeart commented Jan 31, 2022

Looks like #1091 may be related

@NullVoxPopuli
Copy link
Collaborator Author

Gonna reslove the conversations, so we can start up new threads on the up-to-date code

@@ -134,13 +134,45 @@ describe(`dependencySatisfies`, function () {
expect(runDefault(code)).toBe(true);
});

test(`it considers a project's used pre-releases as allowed`, () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these tests pass without my changes to the src directory 😱

@@ -181,3 +181,46 @@ dummyAppScenarios
});
});
});

appReleaseScenario
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hoping to re-create what I'm seeing in my actual app with this scenario test

@NullVoxPopuli
Copy link
Collaborator Author

Trying to run tests locally using the javascript debug terminal. Getting:

❯ yarn test --filter "embroider/util"
Debugger attached.
yarn run v1.22.5
$ qunit --require ts-node/register *-test.ts --filter embroider/util
Debugger attached.
TAP version 13

and then in the debug console:

ARNING in ../../home/psego/Development/NullVoxPopuli/embroider-3/node_modules/ember-data-latest/node_modules/ember-inflector/index.js 3:0-48
export 'defaultRules' (reexported as 'defaultRules') was not found in './lib/system' (possible exports: Inflector, pluralize, singularize)
 @ ./assets/app-template.js 524:13-147

does that seem familiar to anyone?

@NullVoxPopuli
Copy link
Collaborator Author

So, with ember 4.2.0 recently released, I was able to test my app with a stable version of ember, and the problem I'm trying to resolve is still present.
image

dependencyMacros is going down the wrong path here:
image

@rwjblue @ef4 any ideas?

I think this PR (#1080) has to be put on hold until we can get whatever is going on in my app figured out with stable ember.

@ef4
Copy link
Contributor

ef4 commented Feb 8, 2022

  1. Clear your cache in $TMPDIR/embroider to rule that out.
  2. Go to this spot (meaning, the place that's calling dependencySatisfies) in the actual node_modules directories and see what version of ember-source, if any, actually resolves from there. node -e "console.log(require('ember-source/package.json').version)"

@NullVoxPopuli
Copy link
Collaborator Author

  1. no change (whew)
❯ node -e "console.log(require('ember-source/package.json').version)"
Volta error: Could not determine path to project workspace: '../../package.json'

Please ensure that the file exists and is accessible.
Error details written to /home/nullvoxpopuli/.volta/log/volta-error-2022-02-08_11_44_14.688.log
# I deleted the volta entry in the package.json

limber/node_modules/@embroider/util on  upgrade-stuff-2 [$] 
❯ node -e "console.log(require('ember-source/package.json').version)"
4.1.0-alpha.7

This is the wrong version for my app but is relevant to this PR (#1080), so that's good.

I changed the package in my monorepo using 4.1.0-alpha.7 to also use 4.2.0, and ran the node command again:

❯ node -e "console.log(require('ember-source/package.json').version)"
4.2.0

progress. However - this implies that every dependency in a monorepo should be the same version? is there a way around that?
🤔 I wonder if this sort of think would be fixed by yarn 3 or pnpm.
yarn 1 is very buggy.

anywho, my app boots now. thanks!
So, this means I can continue to try to figure the ember-source pre-release scenario for this PR! 🎉

@ef4
Copy link
Contributor

ef4 commented Feb 8, 2022

yarn 1 is very buggy.

It is, but in this case it's buggy because it's respecting Node's node_modules convention, which is the real root of the problem. You'd have the same problem with anything that doesn't replace node_modules. pnpm and yarn 3 are both ways to do that. But I doubt either works with embroider out of the box because @embroider/compat's whole job is rewriting node_modules and it's not going to understand alternative layouts.

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

Successfully merging this pull request may close these issues.

4 participants