I'm against this. This plugin will make work a code which is not
supposed to work and will hide mistakes. Even with all the different
magic, we have in our babel plugins, our code should be a valid JS and
should behave as expected without them.
[70]@andreineculau
This comment has been minimized.
[71]Sign in to view
Copy link (BUTTON) Quote reply
Member Author
I'm not pushing for it, quite the contrary. But it is a nice find
regardless, and if JS would have this pattern built-in (not too mention
have the async stacktraces working!), it is my honest opinion that it
would have made for simpler comprehension and simpler code.
I see an important drawback alone in the fact that you cannot make use
of static type analysis.
PS: 1. code is valid JS (it simply removes the need for you to type
await before every function 2. should behave as expected without them.
= so you'd like to have it as a rule that this repo would not do
anything beyond upcoming JS syntax/behaviour, right? I'm down with that
200%
[75]@IanSavchenko
This comment has been minimized.
[76]Sign in to view
Copy link (BUTTON) Quote reply
Contributor
I would prefer an eslint rule that warns me when there is no await for
an async function. Perhaps babel can do that? At least for
simple/obvious cases.
Relevant: "New rule: enforce async/await consistency"
[85]eslint/eslint#9787
[86]@tobiiasl
This comment has been minimized.
[87]Sign in to view
Copy link (BUTTON) Quote reply
Perhaps this is a good course of actions to avoid that devs forget to
add await before calls to functions returning promises.
1. Code style agreement that all functions returning a promise should
be prefixed with async_.
2. Implement eslint rule that enforces that all async functions are
prefixed with async_.
3. Implement eslint rule that all functions that are await:ed for are
prefixed with async.
4. Start using TypeScript which makes function return type obvious.
5. Implement eslint rule that enforces functions returning a promise
to be prefixed with async_.
6. Enforce the no-floating-promises tslint rule.
[97]@andreineculau [98]andreineculau closed this [99]May 14, 2019
text dump at https://raw.githubusercontent.com/rokmoln/zz-issues/master/tobiipro/babel-preset-firecloud/9.txt
set the same semver-range when installing peer deps. #9
f/anu-fix-semver-range
master f/anu-fix-semver-range
@andreineculau
[45]andreineculau opened this pull request
about 1 year ago
Labels
[46]bug
ref [47]tobiipro/eslint-config-firecloud#19
[48]andreineculau added a commit [49]about 1 year ago
[50]@andreineculau [51]set the same semver-range when installing peer
deps. ref [52]tobiipro/esl…
andreineculau added the [53]bug label [54]about 1 year ago
andreineculau self-assigned this [55]about 1 year ago
andreineculau requested a review from IanSavchenko [56]about 1 year ago
@IanSavchenko
[57]IanSavchenko reviewed [58]about 1 year ago
[59]npm-install-peer-dependencies
@@ -55,6 +55,10 @@ node -e "
continue
}
package.json)" > package.json
cat <<< "$(npx json "this.devDependencies = this.devDependencies || {}"
< package.json)" > package.json
[60]@IanSavchenko [61]IanSavchenko • [62]about 1 year ago
Copy link
I can guess what it does, but you gonna need to explain to me how it
actually works 😄
[63]@andreineculau [64]andreineculau • [65]about 1 year ago
Copy link
here goes some "bashsplaining" 😂
if you simplify the command : cat <<< "$(foo < bar)" > bar, you have
* > bar - write stdout to file
* < bar - read stdin from file
* cat - is cat
* <<< "" - is a oneline heredoc into stdin
in plain english:
* output to stdout (via cat) the contents of the heredoc, and the
heredoc is actually a command, and then into bar
* the key is the heredoc, because it doesn't stream, thus you buffer
the command output, and only after you process the entire
package.json, you start writing back to package.json
❤️ 1
IanSavchenko reacted with heart emoji
[66]@IanSavchenko [67]IanSavchenko • [68]about 1 year ago
Copy link
Thank you a lot!
👍 1
andreineculau reacted with thumbs up emoji
[69]@andreineculau [70]andreineculau • [71]about 1 year ago
Copy link
actually json has an in-place flag. jq has a beard and knows better.
went for the less cryptic in-place version
[72]tobiipro/eslint-config-firecloud@12310e5
@IanSavchenko
[73]IanSavchenko approved these changes [74]about 1 year ago
andreineculau referenced this pull request from commit [75]6856ff4
[76]about 1 year ago
andreineculau merged commit 6856ff4 into master [77]about 1 year ago
andreineculau deleted the f/anu-fix-semver-range branch [78]about 1
year ago
__filename is available in node and webpack, and it would provide an
absolute path, instead of a scoped one
[65]@andreineculau [66]andreineculau added the [67]enhancement label
[68]Feb 24, 2019
[69]@andreineculau
This comment has been minimized.
[70]Sign in to view
Copy link (BUTTON) Quote reply
Member Author
actually maybe it should switch to supporting only __filename, if it
exists - as it will be less confusing, and always right
[74]andreineculau added a commit that referenced this issue [75]Feb 24,
2019
[76]@andreineculau
[77]use __filename in src-arg plugin. [78]fix [79]#10
Verified
This commit was signed with a verified signature.
[80]andreineculau Andrei Neculau
GPG key ID: 79FA7EE650BF9A61 [81]Learn about signing commits
[82]ce50b6b
[83]andreineculau added a commit that referenced this issue [84]Feb 25,
2019
[85]@andreineculau
[86]use __filename in src-arg plugin. [87]fix [88]#10 (BUTTON) …
Verified
This commit was signed with a verified signature.
[89]andreineculau Andrei Neculau
GPG key ID: 79FA7EE650BF9A61 [90]Learn about signing commits
[91]5bc88ee
Conflicts:
plugins/babel-plugin-firecloud-src-arg.js
[92]@andreineculau [93]andreineculau closed this in [94]5071fa3 [95]Feb
25, 2019
[96]andreineculau added a commit that referenced this issue [97]Feb 25,
2019
[98]@andreineculau
[99]Merge pull request [100]#11 [101]from tobiipro/f/anu-filename-src
(BUTTON) …
Verified
This commit was created on GitHub.com and signed with a verified
signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 [102]Learn about signing commits
[103]bae52ee
use __filename in src-arg plugin. fix [104]#10
[105]andreineculau added a commit that referenced this issue [106]Feb
25, 2019
[107]@andreineculau
[108]use __filename in src-arg plugin. [109]fix [110]#10 (BUTTON) …
Verified
This commit was signed with a verified signature.
[111]andreineculau Andrei Neculau
GPG key ID: 79FA7EE650BF9A61 [112]Learn about signing commits
[113]8d7b873
Jest mocks do not allow accessing vars from the outer scope, which
exports is. Also, other weird errors occur, which I don't really want
to dig into. Already wasted too much time on this...
One of the most simple and clean solutions IMO is to use different envs
for dev and test in .babelrc. I managed to get it working pretty easy,
since jest sets NODE_ENV=test and this one is used by Babel to pick
config. We have this variable in our makefile, but it's not exported,
so gets redefined by jest. Another level of control is to use BABEL_ENV
variable, which has higher priority.
Let's discuss in the office how we want to tweak these vars to make it
work properly.
[65]@IanSavchenko [66]IanSavchenko self-assigned this [67]Apr 7, 2018
[68]@andreineculau
This comment has been minimized.
[69]Sign in to view
Copy link (BUTTON) Quote reply
Member
I understand only partially what you're experiencing
* trigger
[78]https://github.com/facebook/jest/tree/6ee2a14b83393c9e3e3408beb
5c4848489f04cf6/packages/babel-plugin-jest-hoist
* and you presumably mock an entire module e.g. aws-sdk while
referencing a shared top-level function, which babel rewrites as
exports.
* which can be fixed by the documented doMock function
[79]facebook/jest#2567 (comment)
Footnotes:
1. The problem is not in the transpilation of the src folder but the
test folder, so adding a .babelrc inside the test disabling
export-all should work
2. I don't even know how come you trigger the export-all plugin inside
a jest test module (which should have no export statements)
[80]@IanSavchenko
This comment has been minimized.
[81]Sign in to view
Copy link (BUTTON) Quote reply
Contributor Author
Okay, so it went a bit deeper than I thought...
Will reply to your quotes first.
trigger
[85]https://github.com/facebook/jest/tree/6ee2a14b83393c9e3e3408beb5
c4848489f04cf6/packages/babel-plugin-jest-hoist
and you presumably mock an entire module e.g. aws-sdk while
referencing a shared top-level function, which babel rewrites as
exports.
which can be fixed by the documented doMock function
[86]facebook/jest#2567 (comment)
This can help sometimes when you don't need the hoisting mock behavior,
but sometimes you need hoisting (you import tested module and it
imports a mocked dependency). Cool thing is that jest allows to have
vars prefixed with mock to be referenced in mocks. This kind of implies
that you know what you are doing. So on our side in export-all we could
add an option for this like ignorePrefix.
The problem is not in the transpilation of the src folder but the
test folder, so adding a .babelrc inside the test disabling
export-all should work
This works, but only for files in the test folder. Files in src folder
will be transpiled with root .babelrc. With suggested workaround above,
we could even keep one plugin config (root) across src and test.
(Spoiler: further, you will see why it's not enough...)
I don't even know how come you trigger the export-all plugin inside
a jest test module (which should have no export statements)
This exception we have only for linter rules, remember? For babel
plugin, I don't think we should do this, because you never know if the
state of AST tree and ExportDeclaration nodes in it are coming from the
source code or were generated by another plugin. Probably, it is
possible to know by analyzing original source code as string (you have
this info in plugin), but I think it's brittle.
BUT... I was wrong when stated the problem in the title. The original
problem (95% of all errors) is with coverage plugin Istanbul and that
it ruins the AST somehow. I checked the code and they don't appear to
be doing anything wrong, but somehow when the execution comes to our
plugin, the tree is not correct and this causes same error as here
[87]istanbuljs/babel-plugin-istanbul#116. Instead of fixing a bug here,
people actually suggest disabling faulting plugin in test env. Sad.
I really-really don't want to spend any more time on this problem, even
though it's so fun to dig in jest/babel/istanbul 😂 Let's sit in the
office when we are both in good health after sickness and decide
something.
[88]@andreineculau
This comment has been minimized.
[89]Sign in to view
Copy link (BUTTON) Quote reply
Member
I have now read your comment in peace. Let's just say bye-bye to this
coverage crap, I say, but we talk tmrw.
[98]@andreineculau [99]andreineculau added this to To do in [100]Public
(Open Source) [101]May 20, 2019
[102]@andreineculau
This comment has been minimized.
[103]Sign in to view
Copy link (BUTTON) Quote reply
Member
closing as it's a istanbul coverage issue
[107]@andreineculau [108]andreineculau closed this [109]Jul 12, 2019
[110]Public (Open Source) automation moved this from To do to Done
[111]Jul 12, 2019
[112]@andreineculau [113]andreineculau changed the title [DEL:
export-all conflicts with jest mocks :DEL] [INS: export-all conflicts
with istanbul :INS] [114]Jul 12, 2019
[115]@ankitmth [116]ankitmth removed this from Done in [117]Public
(Open Source) [118]Feb 19, 2020
One could argue though that you anyway have to provide srcFuns in order
to make this plugin useful, so it's not a big deal to provide also
disabled: false.
[66]@andreineculau [67]andreineculau added the [68]question label
[69]Feb 23, 2018
[70]@andreineculau [71]andreineculau closed this in [72]adfce97 [73]Mar
23, 2018
Case like let _ = require('lodash-firecloud');
Otherwise, we can get in the end exports._.merge(...
[66]@IanSavchenko [67]IanSavchenko added the [68]bug label [69]Oct 5,
2018
[70]@IanSavchenko [71]IanSavchenko self-assigned this [72]Oct 5, 2018
[73]@IanSavchenko [74]IanSavchenko closed this in [75]e06cee3 [76]Oct
8, 2018
test by building schemata-firecloud, which with @babel/[email protected]
fails with
09:32:23 [INFO] Generating dts/atex-beacon-in-view-event-v1-json.d.ts...
error SyntaxError: Unexpected token W in JSON at position 1
at JSON.parse ()
at /Users/andrei/git/firecloud/schemata-firecloud/node_modules/json-schema-t
o-typescript/dist/src/cli.js:70:33
at step (/Users/andrei/git/firecloud/schemata-firecloud/node_modules/json-sc
hema-to-typescript/dist/src/cli.js:33:23)
at Object.next (/Users/andrei/git/firecloud/schemata-firecloud/node_modules/
json-schema-to-typescript/dist/src/cli.js:14:53)
at fulfilled (/Users/andrei/git/firecloud/schemata-firecloud/node_modules/js
on-schema-to-typescript/dist/src/cli.js:5:58)
[65]@andreineculau [66]andreineculau added the [67]enhancement label
[68]Mar 20, 2019
[69]@andreineculau
This comment has been minimized.
[70]Sign in to view
Copy link (BUTTON) Quote reply
Member Author
upstream issue: [74]babel/babel#9977
[75]@andreineculau [76]andreineculau closed this in [77]015e752 [78]May
14, 2019
[79]andreineculau added a commit that referenced this issue [80]May 14,
2019
[81]@andreineculau
[82]fixup! specify corejs version to silence warning. [83]fixes [84]#14
Verified
This commit was signed with a verified signature.
[85]andreineculau Andrei Neculau
GPG key ID: 79FA7EE650BF9A61 [86]Learn about signing commits
[87]305b1a8
regardless of the state of [65]#2 , we should be consistent with
* exporting or not non-api vars in a module
* marking non-api vars with an underscore prefix or not
PS: this is a TODO for all the repos we now have on github, not a real
issue for this repo
[66]@andreineculau [67]andreineculau added the [68]enhancement label
[69]Mar 15, 2018
[70]@andreineculau
This comment has been minimized.
[71]Sign in to view
Copy link (BUTTON) Quote reply
Member Author
NOTE A negative side-effect. While debugging, Chrome won't be able
to get the value of an exported variable, because the source map
will reference exports.something, while you still hover something.
[67]@andreineculau [68]andreineculau added [69]bug [70]help wanted
labels [71]Feb 23, 2018
[72]@andreineculau [73]andreineculau mentioned this issue [74]Mar 15,
2018
[75]consistent exports/underscore-private #3
Closed
[76]andreineculau added a commit that referenced this issue [77]Mar 23,
2018
[78]@andreineculau
[79]enable export-all. known negative side-effect tracked in [80]#2
Unverified
This user has not uploaded their public key yet.
GPG key ID: 3570C6DE977687AA [81]Learn about signing commits
[82]549d0df
[83]@andreineculau [84]andreineculau mentioned this issue [85]Mar 25,
2018
[86]rule: check not-exported variables are prefixed with _ #6
Closed
[87]@andreineculau [88]andreineculau added this to To do in [89]Public
(Open Source) [90]May 20, 2019
[91]@andreineculau [92]andreineculau changed the title [DEL: fix
babel-plugin-firecloud-export-all :DEL] [INS:
babel-plugin-firecloud-export-all and sourcemaps don't play well in
chrome :INS] [93]Jul 12, 2019
[94]@andreineculau [95]andreineculau added the [96]invalid label
[97]Jul 12, 2019
[98]@andreineculau
This comment has been minimized.
[99]Sign in to view
Copy link (BUTTON) Quote reply
Member Author
dunno if I'm doing smth wrong, but I cannot reproduce this now. marking
as invalid and closing
[103]@andreineculau [104]andreineculau closed this [105]Jul 12, 2019
[106]Public (Open Source) automation moved this from To do to Done
[107]Jul 12, 2019
[108]@ankitmth [109]ankitmth removed this from Done in [110]Public
(Open Source) [111]Feb 19, 2020
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent