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

v1.3.0 (re-release due to violation of semantic versioning) #69

Merged
merged 2 commits into from
Sep 2, 2019

Conversation

FND
Copy link
Contributor

@FND FND commented Aug 2, 2019

v1.2.{1,2} will be deprecated

in the process, updated dependencies and revised changelog

  • ensure new path resolution (details in dc5e207) does not lead to any obvious backwards-compatibility issues (e.g. with faucet-sass - i.e. I hope legacy behavior was never actually being used)
    cf. require.resolve regression in Node v12 with paths nodejs/node#29139 (comment)

  • revise changelog for consistency

  • update release date in changelog

  • release v1.3.0

  • deprecate v1.2.x ("re-released as v1.3.0 due to violation of semantic versioning"?)

    $ npm deprecate faucet-pipeline-core@"~1.2.1" "$msg"
    

@FND FND requested a review from moonglum August 2, 2019 07:25
Copy link
Member

@moonglum moonglum left a comment

Choose a reason for hiding this comment

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

Yep, looks like a good fix 👍

@FND
Copy link
Contributor Author

FND commented Aug 14, 2019

Update: This was an intentional change in Node v12 - see nodejs/node#29139 and dc5e207 for details.

reduced test case:

/path/to/faucet-core/test/fixtures
├── dummy
│   ├── index.js
│   └── src.js
└── node_modules
    └── dummy
        ├── index.js
        └── pkg.js
$ cd /tmp
$ node -e 'require.resolve("dummy/src.js", { paths: ["/path/to/faucet-core/test/fixtures"] })'
Error: Cannot find module 'dummy/src.js'

This only fails on Node v12 though; both v8 and v10 work as expected.

However, I would argue that test (introduced in 3142f2e) makes no sense: Why would dummy/src.js resolve to ./dummy/src.js; that's only supposed to work for ./node_modules/dummy/src.js, no? I'm uncertain whether that was just a case of us being a little careless or whether there's more to this (think backwards compatibility).

@FND FND force-pushed the semver-fix branch 2 times, most recently from c274752 to 95b0d6c Compare August 16, 2019 05:46
@FND FND requested a review from moonglum August 16, 2019 06:54
Copy link
Member

@moonglum moonglum left a comment

Choose a reason for hiding this comment

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

Yes, this behavior was unintentional. Thanks for fixing it! I say: Let's release this.

FND added 2 commits September 2, 2019 07:36
`require.resolve` behavior was changed in Node v12 to comply with Node
resolution algorithm (see nodejs/node#29139) -
not sure why we thought this behavior was desirable in the first place
(introduced in 3142f2e - probably
cargo-culting `require.resolve`'s observed behavior)

however, on legacy Node versions behavior appears unchanged if the
current working directory is the same as `rootDir` - I didn't bother
enforcing modern behavior because legacy isn't worth the effort

in the process, fixed legacy detection
v1.2.{1,2} will be deprecated

in the process, upgraded dependencies and revised changelog
@FND FND merged commit d355e34 into master Sep 2, 2019
@FND FND deleted the semver-fix branch September 2, 2019 05:40
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.

2 participants