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

Is there a reason magic-string isn't bundled? #410

Closed
Conduitry opened this issue Mar 27, 2017 · 3 comments
Closed

Is there a reason magic-string isn't bundled? #410

Conduitry opened this issue Mar 27, 2017 · 3 comments

Comments

@Conduitry
Copy link
Member

Adjusting the Rollup config so that magic-string isn't an external dependency and is instead bundled along with everything else seems to work perfectly fine. Is there a particular reason that one dependency excluded from the bundle?

@Conduitry
Copy link
Member Author

diff --git a/package.json b/package.json
index 2f80203..c1022be 100644
--- a/package.json
+++ b/package.json
@@ -39,9 +39,7 @@
     "url": "https://github.com/sveltejs/svelte/issues"
   },
   "homepage": "https://github.com/sveltejs/svelte#README",
-  "dependencies": {
-    "magic-string": "^0.19.0"
-  },
+  "dependencies": {},
   "devDependencies": {
     "acorn": "^4.0.4",
     "babel": "^6.23.0",
@@ -65,6 +63,7 @@
     "glob": "^7.1.1",
     "jsdom": "^9.9.1",
     "locate-character": "^2.0.0",
+    "magic-string": "^0.19.0",
     "mocha": "^3.2.0",
     "node-resolve": "^1.3.3",
     "nyc": "^10.0.0",
diff --git a/rollup/rollup.config.main.js b/rollup/rollup.config.main.js
index 334ead6..5b03263 100644
--- a/rollup/rollup.config.main.js
+++ b/rollup/rollup.config.main.js
@@ -21,9 +21,5 @@ export default {
                        }
                })
        ],
-       external: [ 'magic-string' ],
-       globals: {
-               'magic-string': 'MagicString'
-       },
        sourceMap: true
 };
diff --git a/rollup/rollup.config.ssr.js b/rollup/rollup.config.ssr.js
index 6d16a18..832b9b0 100644
--- a/rollup/rollup.config.ssr.js
+++ b/rollup/rollup.config.ssr.js
@@ -20,7 +20,7 @@ export default {
                        }
                })
        ],
-       external: [ path.resolve( 'src/index.js' ), 'fs', 'path', 'magic-string' ],
+       external: [ path.resolve( 'src/index.js' ), 'fs', 'path' ],
        paths: {
                [ path.resolve( 'src/index.js' ) ]: '../compiler/svelte.js'
        },

@PaulBGD
Copy link
Member

PaulBGD commented Mar 27, 2017

It's just 1 additional module to be downloaded, so if NPM/yarn already has it in the cache then it's a quicker download.

@Rich-Harris
Copy link
Member

I was wondering this same question... I think it probably should be bundled — it means installation is a bit quicker (might just be one extra cached module, or two since magic-string has an external dependency of its own, but it still makes a difference particularly in a CI context) and it's easier to use the compiler in esoteric situations such as the REPL (where we currently have to faff around with AMD config).

In short, if our policy is to bundle dependencies to create the fastest possible startup experience, the leanest possible installation, and the most convenient possible distributable (and it is 😀 ) then there's no obvious reason magic-string should be excluded. It was clearly done intentionally but I have no idea what the intention was! I reckon we should bundle it and see what happens.

Rich-Harris added a commit that referenced this issue Mar 27, 2017
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

No branches or pull requests

3 participants