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

Does not play nice with package layouts #8

Open
CosineP opened this issue Jul 15, 2021 · 0 comments
Open

Does not play nice with package layouts #8

CosineP opened this issue Jul 15, 2021 · 0 comments

Comments

@CosineP
Copy link

CosineP commented Jul 15, 2021

The following diff will make matchkw work with anchor/ide:

--- a/addon/matchkw.js
+++ b/addon/matchkw.js
@@ -1,8 +1,8 @@
 (function(mod) {
   if (typeof exports == "object" && typeof module == "object") // CommonJS
-    mod(require("../../lib/codemirror"), require("../fold/pyret-fold"));
+    mod(require("../../codemirror"), require("pyret-fold"));
   else if (typeof define == "function" && define.amd) // AMD
-    define(["../../lib/codemirror", "../fold/pyret-fold"], mod);
+    define(["../../codemirror", "pyret-fold"], mod);
   else // Plain browser env
     mod(CodeMirror);
 })(function(CodeMirror) {
diff --git a/addon/pyret-fold.js b/addon/pyret-fold.js
index 7413249..638db02 100644
--- a/addon/pyret-fold.js
+++ b/addon/pyret-fold.js
@@ -1,8 +1,8 @@
 (function(mod) {
   if (typeof exports == "object" && typeof module == "object") // CommonJS
-    mod(require("../../lib/codemirror"));
+    mod(require("../../codemirror"));
   else if (typeof define == "function" && define.amd) // AMD
-    define(["../../lib/codemirror"], mod);
+    define(["../../codemirror"], mod);
   else // Plain browser env
     mod(CodeMirror);
 })(function(CodeMirror) {

But obviously at some point, the lib/ path was relevant. (My guess is that pyret-codemirror-mode was once installed in codemirror/addon, which i'm not sure why or how that would happen.) That means to me that this "path guessing" is not a good approach. That said, i don't think this would interfere with CPO because looking at the usage, it appears that it's following the // Plain browser env path, so for an easy fix this is an option.

Alternatively, we could keep working on PR #6, because a peer dependency does seem to be the more 'principled' way to do this (and react-codemirror2 specifically warns that non-peer dependencies on CodeMirror can cause really subtle bugs).

Finally, we could check if (CodeMirror) first, therefore allowing anyone to use the window.CodeMirror hack if the module directories aren't exactly as they should be.

Thoughts on the right approach?

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

1 participant