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

Add tslib to installation instructions #253

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

brandon-leapyear
Copy link
Contributor

@brandon-leapyear brandon-leapyear commented Nov 18, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


Version 2 of yarn uses Plug'n'Play, which means that the user's installed typescript (which is used because typescript is set as a peer dependency for this plugin) won't have access to the tslib marked as a dependency for this plugin.

yarnpkg/berry#2140

People using version 2 of yarn have to manually install tslib in devDependencies. This PR makes it so that everyone would need to manually install tslib as well, which is what should actually happen.

@merceyz
Copy link

merceyz commented Nov 18, 2020

This isn't Yarn PnP specific, it's required for all package managers, PnP just enforces it

@brandon-leapyear brandon-leapyear changed the title Add tslib to installation instructions for yarn Add tslib to installation instructions and peer dependency Nov 18, 2020
@ezolenko
Copy link
Owner

What happens currently? Does typescript pull its own version of tslib and plugin pulls in its own?

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 20, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


Currently, because rollup-plugin-typescript has tslib as a dependency, it installs it in node_modules, and normal npm usage would find it, because npm module resolution just looks for the existence of a node_modules directory and sees if node_modules/tslib/package.json exists.

However, this is actually relying on incorrect behavior on npm's part, and yarn v2 fixes this by using a scheme called Plug'n'Play, where it doesn't use node_modules at all and instead explicitly exposes packages only if the packages should be exposed to the running process. If a user doesn't explicitly install tslib, Yarn v2 will error saying that the rollup build process can't find tslib.

Because rollup-plugin-typescript2 doesn't actually use tslib (i.e. there's no require('tslib') or import ... from 'tslib' in rollup-plugin-typescript2), it shouldn't be a dependency, but actually a peerDependency, which indicates that rollup-plugin-typescript2 doesn't directly use it but it's transitively required at runtime (when using the typescript library)

@ezolenko
Copy link
Owner

Hm, the plugin actually uses tslib at runtime here

const tslibPath = require.resolve("tslib/" + tslibPackage.module);

Will that cause problems? Should we add tslib to both peer dependencies and dependencies?

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 20, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


Interesting! Ok, then let's move it back to dependencies. I think, maybe, the typescript library should list tslib as a peerDependency, since the typescript library expects tslib to already be installed (but it's not listed in typescript's dependencies)

@merceyz do you have any thoughts on this?

@brandon-leapyear brandon-leapyear changed the title Add tslib to installation instructions and peer dependency Add tslib to installation instructions Nov 20, 2020
@merceyz
Copy link

merceyz commented Nov 20, 2020

What happens currently? Does typescript pull its own version of tslib and plugin pulls in its own?

@ezolenko When you enable importHelpers TypeScript checks your project for tslib but since it's not added as a dependency PnP is rejecting access to it and TypeScript throws.

To illustrate this problem without PnP imagine for a moment there is no hoisting and you haven't added tslib as a dependency to your project, your node_modules layout will look like this:

package.json
src
  -> index.ts
node_modules
  -> typescript
  -> rollup
  -> rollup-plugin-typescript2
     -> node_modules
        -> tslib

Now when TypeScript has to import a helper from tslib and add it to src/index.ts it will check if it can find it from that file, which it can't.

@merceyz do you have any thoughts on this?

@brandon-leapyear The solution here is for the user to add tslib to their project, how that's communicated is up to @ezolenko

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 20, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


@merceyz Thanks! My question is more, is the "correct" solution to get typescript to add tslib as a dependency/peerDependency`?

@ezolenko
Copy link
Owner

@merceyz

Now when TypeScript has to import a helper from tslib and add it to src/index.ts it will check if it can find it from that file, which it can't.

Hm, this shouldn't be a problem unless typescript is used directly. Plugin resolves tslib itself, so when typescript tries to import it, it will generate import statement in js code (because of importHelpers: true override in tsconfig), plugin will resolve that to its internal copy that it loaded in src/tslib.ts and rollup will inject it into the bundle correctly.

Does it break somehow?

@merceyz
Copy link

merceyz commented Nov 20, 2020

@merceyz Thanks! My question is more, is the "correct" solution to get typescript to add tslib as a dependency/peerDependency`?

The correct solution is for the user to add tslib to their dependencies, adding it as dependency to typescript wouldn't help (ref #253 (comment)) and if it was a peer dependency it would be optional so you wouldn't see a warning until it failed at build time anyways.

Hm, this shouldn't be a problem unless typescript is used directly. Plugin resolves tslib itself, so when typescript tries to import it, it will generate import statement in js code (because of importHelpers: true override in tsconfig), plugin will resolve that to its internal copy that it loaded in src/tslib.ts and rollup will inject it into the bundle correctly.

Does it break somehow?

As I said, TypeScript will check if it's installed before injecting it, so the plugin changing it later doesn't matter.

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 20, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


When you enable importHelpers TypeScript checks your project for tslib

Would this be solved if TypeScript's check for tslib did the equivalent of require.resolve('tslib')?

@merceyz
Copy link

merceyz commented Nov 20, 2020

That would still make you responsible for adding it as a dependency, it's not whether or not typescript can find it, it's if your project can.

@brandon-leapyear
Copy link
Contributor Author

brandon-leapyear commented Nov 20, 2020

This is an old work account. Please reference @brandonchinn178 for all future communication


What do you mean? Isn't this error coming about when typescript is compiling a .ts file and needs to (effectively) copy and paste tslib into the compiled JS? Or are you saying that typescript just injects an import "tslib" statement and separately checks to see if that would fail at runtime?

@ezolenko
Copy link
Owner

ezolenko commented Nov 20, 2020

I think he is saying typescript itself will try to find tslib by its own means before generating import statement when transpiling ts. So it works if ts either doesn't check and just generate import (plugin will handle it), or it checks and finds it.

@ezolenko
Copy link
Owner

ezolenko commented Nov 20, 2020

PR is just a readme change now, I can merge it unless somebody things something else needs to be done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc scope: docs Documentation could be improved. Or changes that only affect docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants