fix: do not read configuration outside of the root - #769

Merged
thymikee merged 1 commit into
masterfrom
fix/config-paths
Oct 8, 2019
Merged

fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom
fix/config-paths

Conversation

@grabbou

@grabbougrabbou commented Oct 3, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Summary:

Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.

Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.

Context:

Fixes#655).

As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.

I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).

Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.

@grabbougrabbou mentioned this pull request Oct 3, 2019
Closed
@grabbou

grabbou commented Oct 3, 2019

Copy link
Copy Markdown
MemberAuthor

I believe that the test failures are unrelated.

@tbergquist-godaddy

tbergquist-godaddy commented Oct 4, 2019

Copy link
Copy Markdown

I believe that the test failures are unrelated.

On #749 this test also failed randomly.

@tbergquist-godaddy

tbergquist-godaddy commented Oct 4, 2019

Copy link
Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.
This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂

If you want to see my setup, I have a monorepo here.

Still, the bundler cannot find the react-navigation back image.

@grabbou

grabbou commented Oct 4, 2019

Copy link
Copy Markdown
MemberAuthor

@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.

@tbergquist-godaddy

tbergquist-godaddy commented Oct 4, 2019

Copy link
Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely

@grabbou

grabbou commented Oct 8, 2019

Copy link
Copy Markdown
MemberAuthor

Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.

This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.

thymikee
thymikee approved these changes Oct 8, 2019
View reviewed changes

@thymikeethymikee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's do it

@thymikeethymikee changed the title Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee merged commit 71f109e into masterOct 8, 2019
@thymikee
thymikee deleted the fix/config-paths branch October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object

3 participants

@grabbou@tbergquist-godaddy@thymikee
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
fix: do not read configuration outside of the root - #769
Merged
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Oct 8, 2019
Merged
fix: do not read configuration outside of the root#769
thymikee merged 1 commit into
masterfrom 
fix/config-paths
Conversation
@grabbou
@grabbougrabbou
commented
Oct 3, 2019•

edited

Loading

Copy link


Copy Markdown

Member
Summary:
Reading configuration outside of the current working directory (in other words, project root), is not supported and can lead to dangerous errors. I was pretty sure we never supported that, until I've found this out today.
Since I was able to remove this functionality without changing any tests, my recommendation is to not include it until: a) there is such a requirement (validated) from the user-side and that b) we have thoroughly tested this behavior.
Context:
Fixes#655).
As per a comment from the above issue, the configuration is placed at the root of the repository whereas the CLI is run from the root of the project (which is different and resolves to ./packages/mobile). That tiny difference is crucial and because paths in the configuration are always resolved relative to the root of the project, it creates silent issues that are hard to debug.
I believe it's a good practice to require running the CLI from the root of the mobile project (where react-native is defined in the package.json, just like Metro already does).
Few workarounds relied on this mechanism to provide support for mono-repos (it was broken anyway). However, the PR #768 is going to address these cases by rolling out a new mechanism for this.
@grabbougrabbou
mentioned this pull request
Oct 3, 2019
 Closed

@grabbou
grabbou
commented
Oct 3, 2019

Copy link


Copy Markdown

MemberAuthor
I believe that the test failures are unrelated.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

I believe that the test failures are unrelated.
On #749 this test also failed randomly.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, this solves half the problem. It crashes because of def command = "node ./node_modules/react-native/cli.js config" <- this line in node_modules.gradle.

This is simply not where that module is installed, after changing it to def command = "node ../../node_modules/react-native/cli.js config" it all works as expected 🙂
If you want to see my setup, I have a monorepo here.
Still, the bundler cannot find the react-navigation back image.
@grabbou
grabbou
commented
Oct 4, 2019

Copy link


Copy Markdown

MemberAuthor
@tbergq exactly, this is what I am tackling in the other PR - #768. On iOS, I have resolved that issue. Working on Android now.
@tbergquist-godaddy
tbergquist-godaddy
commented
Oct 4, 2019

Copy link


Copy Markdown

Ok, thank you @grabbou 🙂 I will follow both PR's closely
@grabbou
grabbou
commented
Oct 8, 2019

Copy link


Copy Markdown

MemberAuthor
Up, @thymikee, can I count on your review & merge? This is pretty important for the other PR to ensure there's no confusion while setting things up.
This is a small breaking change. It alters the undocumented behaviour, but changes setup for monorepositories which I believe could've been quite popular with this workaround.
thymikee
thymikee
approved these changes
Oct 8, 2019
View reviewed changes
@thymikeethymikee
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Let's do it
@thymikeethymikee
changed the title
Do not read configuration outside of the rootfix: do not read configuration outside of the rootOct 8, 2019
@thymikee
thymikee
merged commit 71f109e
into
masterOct 8, 2019
@thymikee
thymikee
deleted the

fix/config-paths

branch
October 8, 2019 20:12
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@thymikeethymikeethymikee approved these changes
@EsemesekEsemesekAwaiting requested review from Esemesek

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Cannot get property 'packageName' on null object


3 participants

@grabbou@tbergquist-godaddy@thymikee