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

Use user's typescript first, fallback to bundled #741

Merged
merged 5 commits into from
Dec 17, 2020

Conversation

whitetrefoil
Copy link
Contributor

This PR is aimed at fixing #711.

Microbundle will load "typescript" from path.dirname(process.argv[1]).

If no process.argv[1], or failed to resolve "typescript" from there, fallback to the bundled "typescript".

P.S.
Using [email protected] instead of the latest v5.0.0 because v4.0.0 has already been in package-lock.json.

@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2020

🦋 Changeset detected

Latest commit: 32d8056

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
microbundle Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wardpeet
Copy link
Collaborator

@whitetrefoil, could you explain a little what the origin is of this feature? I bet you want to use typescript 4.0 and we don't support it yet?

@whitetrefoil
Copy link
Contributor Author

@whitetrefoil, could you explain a little what the origin is of this feature? I bet you want to use typescript 4.0 and we don't support it yet?

Yes, you're correct. See #711 where has 2 examples there: jsxFragmentFactory and catch (e: unknown).

BTW, IMO use the ts version of the project to be bundled should make more sense than using a fixed version.

Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

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

Looks good! I think we can simplify this down to basically the following:

require(resolveFrom.silent(options.cwd, 'typescript') || 'typescript'))

... but otherwise good to go.

src/index.js Outdated Show resolved Hide resolved
@@ -9,6 +9,7 @@ import autoprefixer from 'autoprefixer';
import cssnano from 'cssnano';
import { rollup, watch } from 'rollup';
import builtinModules from 'builtin-modules';
import resolveFrom from 'resolve-from';
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this instead of just require.resolve and will this break with something like yarn pnp / pnpm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not much different in theory. "resolve-from" needs node v8.0 while paths options of require.resolve requires node v8.9. And "resolve-from" support .silent which will lead to a bit nicer code in this case.

This PR works with yarn, I'm using yarn actually. But I have no idea about pnpm.

BTW there's an issue in "require-resolve" which complained that paths of require.resolve doesn't work. But I never dug deeper about that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Node 8 is EOL so that doesn't matter

You're using yarn 2? Then it's fine to merge

@@ -0,0 +1,5 @@
---
'microbundle': minor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you consider this a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used to considered it to be a fix + feature instead of break, and it can fallback to previous behavior.

After thought more carefully... Maybe you're right, in theroy if user previously used different TS version during developing & bundling, it will lead to different result.

But I'm still hesitating. Logically if a user previously:

  • used same version in dev & bundling - no change
  • verA in dev + verB in bundling, both worked - use verA in both stages should also work
  • verA in dev + verB in bundling, OK in dev but failed to bundle - this PR means a fix i.o. a break to this user
  • verA in dev + verB in bundling, OK in bundling but failed in dev - this PR is a break, but I'm not sure if this just happen...

Anyway, major is always a safer choice... Shall I update the PR?

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