- Notifications
You must be signed in to change notification settings - Fork 13
fix #18#19
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
fix #18 #19
Uh oh!
There was an error while loading. Please reload this page.
Conversation
caub commented Feb 13, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
1c1c21e to b506512Compareeriwen commented Sep 14, 2019
caub commented Sep 15, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@eriwen I added a commit with a test, but |
niftylettuce commented Sep 16, 2019 via email
It (primordials) wasn't added until Node 10 or something I think. Tests run on 8 right now. Not at desk but can look later. …On September 15, 2019 7:05:47 PM UTC, Cyril Auburtin ***@***.***> wrote: @eriwen I add this test, but `npm test` isn't working for me, there's probably something to install ``` > gulp test fs.js:27 const{Math, Object } = primordials; ^ ReferenceError: primordials is not defined at fs.js:27:26 at req_ (/home/caub/dev/stackframe/node_modules/natives/index.js:143:24) at Object.req [as require] (/home/caub/dev/stackframe/node_modules/natives/index.js:55:10) at Object.<anonymous> (/home/caub/dev/stackframe/node_modules/vinyl-fs/node_modules/graceful-fs/fs.js:1:37) ``` -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #19 (comment) |
caub commented Sep 16, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I added a fix commit after googling https://stackoverflow.com/a/55926692/3183756 and gulpjs/undertaker#54 (comment) (I'm using node12, with this commit it should support all node versions) ps: btw I'd rather replace all those gulp tasks by simple npm scripts |
| }, done).start(); | ||
| }); | ||
| gulp.task('copy', function(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved up, the task must be defined before use
gulpfile.js Outdated
| }); | ||
| gulp.task('dist', ['copy'], function(){ | ||
| gulp.task('dist', gulp.series(['copy'], function(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
niftylettuce commented Sep 17, 2019
@caub What's left for this PR to do? Do we need to upgrade to Gulp 4.x and rewrite gulpfile.js? Or switch to NPM scripts? If we do it one way or another, we need to make it consistent across the entire org. |
caub commented Sep 17, 2019
I upgraded gulp to 4.x on this repo to be able to run tests locally, I can left it to 3.x if you prefer |
niftylettuce commented Sep 17, 2019
@caub Could you also upgrade all other repos to 4.x? Would be immensely helpful. I know I'm asking a lot, if you don't have the time I could try to squeeze some in this weekend. |
caub commented Sep 17, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Sorry, I don't want to rewrite anything else to gulp4, I could rather remove gulp on this repo (and use npm scripts), for other repos I think it can be incremental as it's not breaking anything, it's just a devDep (so no need to do it all at once) |
caub commented Sep 18, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
So what do you choose?
|
niftylettuce commented Sep 18, 2019
npm scripts would be awesome |
caub commented Sep 20, 2019
@niftylettuce ok, great idea, I'll make another PR for this later I've removed the gulp4 commit for this PR, to keep it on topic, and merged |
eriwen commented Jan 5, 2020
Thanks for your contribution, @caub. I now have some time to give stacktrace.js some love and have merged your PR. |
Hey @eriwen, could yo have a look?