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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f81c3b3
use process.cwd()'s typescript
whitetrefoil Oct 16, 2020
0a0aa0d
use process.argv[1]'s instead of process.cwd()'s typescript
whitetrefoil Oct 16, 2020
aa06df4
Handle situation that argv1 is empty
whitetrefoil Oct 16, 2020
82df6ab
changeset
whitetrefoil Oct 16, 2020
32d8056
Require "typescript" from cwd i.o. argv1
whitetrefoil Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silver-guests-brake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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?

---

Use user's typescript first, fallback to bundled
9 changes: 8 additions & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
"lodash.merge": "^4.6.2",
"module-details-from-path": "^1.0.3",
"pretty-bytes": "^5.3.0",
"resolve-from": "^4.0.0",
"rollup": "^1.32.1",
"rollup-plugin-bundle-size": "^1.0.1",
"rollup-plugin-es3": "^1.1.0",
Expand Down
15 changes: 14 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

import commonjs from '@rollup/plugin-commonjs';
import babel from '@rollup/plugin-babel';
import customBabel from './lib/babel-custom';
Expand Down Expand Up @@ -402,6 +403,18 @@ function createConfig(options, entry, format, writeMeta) {
const outputDir = dirname(absMain);
const outputEntryFileName = basename(absMain);

const argv1 = process.argv[1];
let typescriptLibPath;
if (argv1 != null) {
// If argv1 exists, try to require 'typescript' from there.
typescriptLibPath = resolveFrom(dirname(argv1), 'typescript');
}
if (typescriptLibPath == null) {
// If argv1 doesn't exist or no 'typescript' found there,
// fallback to bundled 'typescript'.
typescriptLibPath = 'typescript';
}

let config = {
/** @type {import('rollup').InputOptions} */
inputOptions: {
Expand Down Expand Up @@ -486,7 +499,7 @@ function createConfig(options, entry, format, writeMeta) {
},
useTypescript &&
typescript({
typescript: require('typescript'),
typescript: require(typescriptLibPath),
cacheRoot: `./node_modules/.cache/.rts2_cache_${format}`,
useTsconfigDeclarationDir: true,
tsconfigDefaults: {
Expand Down