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

fix(taskr): use createRequire to perform relative requires and add missing optional peer dependency @taskr/esnext #317

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

merceyz
Copy link

@merceyz merceyz commented Sep 23, 2020

What's the problem this PR addresses?

taskr makes assumptions about the node_modules layout and tries to require files directly from it instead of using createRequire, taskr also has an optional peer dependency on @taskr/esnext that it doesn't declare making it rely heavily on hoisting to place it in an accesible location.

Read https://yarnpkg.com/advanced/rulebook for more in depth information

How did you fix it?

cc @lukeed @arcanis

Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Thank you, but I think just defining peerDep is all that's needed. Installers will preserve access to the taskr/esnext

@merceyz merceyz changed the title fix(taskr): perform actual relative require and add missing dependency @taskr/esnext fix(taskr): perform actual relative require and add missing optional peer dependency @taskr/esnext Sep 23, 2020
@merceyz
Copy link
Author

merceyz commented Sep 23, 2020

I think just defining peerDep is all that's needed. Installers will preserve access to the taskr/esnext

Do you mean just making @taskr/esnext a normal peerDependency, not optional?

@lukeed
Copy link
Owner

lukeed commented Sep 23, 2020

No, still optional. You already have it listed as a peerDependency. That's enough to have taskr and @taskr/esnext linked if/when @taskr/esnext is installed.

@merceyz
Copy link
Author

merceyz commented Sep 23, 2020

Do you want me to remove the createRequire part? Can't really do that as it's needed for all the other plugins it attempts to access, not @taskr/esnext

@lukeed
Copy link
Owner

lukeed commented Sep 23, 2020

Can you please include or list steps for a simple reproduction?

@merceyz
Copy link
Author

merceyz commented Sep 24, 2020

Sure, here is a sh file that will reproduce the issue

mkdir taskr-repro
cd taskr-repro

printf "exports.lint = function * (task) {
  yield task.source('src/**/*.js').prettier({
    semi: false,
    useTabs: true,
    trailingComma: 'es5'
  }).target('dist/js');
}" > taskfile.js

mkdir src
printf "console.log('foo');    // bar" > src/index.js

yarn init -y2p
yarn config set pnpFallbackMode none
yarn add taskr @taskr/prettier
PNP_DEBUG_LEVEL=1 yarn taskr lint

The issue is that taskr tries to require @taskr/prettier from itself, and then directly from /taskr-repro/node_modules/@taskr/prettier both are invalid because taskr can be hoisted higher up than @taskr/prettier and /taskr-repro/node_modules/@taskr/prettier isn't guaranteed to exist.

The solution is to perform the require relative to the users project which declares the plugins.

https://yarnpkg.com/advanced/rulebook#modules-shouldnt-hardcode-node_modules-paths-to-access-other-modules

@merceyz merceyz changed the title fix(taskr): perform actual relative require and add missing optional peer dependency @taskr/esnext fix(taskr): use createRequire to perform relative requires and add missing optional peer dependency @taskr/esnext Sep 24, 2020
@merceyz
Copy link
Author

merceyz commented Oct 3, 2020

@lukeed Anything else you'd like me to do to move this forward?

@lukeed
Copy link
Owner

lukeed commented Oct 6, 2020

Hey, sorry for the delay on this! Never got around to setting up yarn@2 (don't want to alter my environment)

Should't something like this work too? I have something like this in other places:

function req(name, base) {
  try {
    return require( require.resolve(name, { paths: [base] }) );
  } catch (e) {
    $.alert(e.message);
  }
}

@merceyz
Copy link
Author

merceyz commented Oct 6, 2020

Hey, sorry for the delay on this!

No worries :)

Never got around to setting up yarn@2 (don't want to alter my environment)

Yarn@2 is installed on a per project basis so you would only alter a single project, the repro I provided does just that in the init command.

Should't something like this work too? I have something like this in other places:

Most likely yes, it's only in some rare cases where it doesn't quite work. However, since taskr is set to target >[email protected][1] and the paths option wasn't added until [email protected][2] would need a polyfill anyways.

1:

"node": ">= 4.6"

2: https://nodejs.org/api/modules.html#modules_require_resolve_request_options

@merceyz
Copy link
Author

merceyz commented Nov 16, 2020

@lukeed Sorry to ping again but would be nice to get this merged

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