GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix #18 by caub · Pull Request #19 · stacktracejs/stackframe

Conversation

@caub

@caub caub commented Feb 13, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hey @eriwen, could yo have a look?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@caub
caub force-pushed the patch-1 branch 3 times, most recently from 1c1c21e to b506512 Compare February 20, 2019 21:09
@eriwen
eriwen self-requested a review September 14, 2019 21:49
@eriwen eriwen self-assigned this Sep 14, 2019
@eriwen

eriwen commented Sep 14, 2019

Copy link
Copy Markdown
Member

Hey @caub. Could you please add a test that ensures #18 stays fixed?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caub

caub commented Sep 15, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@eriwen I added a commit with a 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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@niftylettuce

niftylettuce commented Sep 16, 2019 via email

Copy link
Copy Markdown
Contributor
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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caub

caub commented Sep 16, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
caub
caub commented Sep 16, 2019
View reviewed changes
Comment thread gulpfile.js
}, done).start();
});

gulp.task('copy', function() {

@caub caub Sep 16, 2019

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
caub
caub commented Sep 16, 2019
View reviewed changes
Comment thread gulpfile.js Outdated
});

gulp.task('dist', ['copy'], function() {
gulp.task('dist', gulp.series(['copy'], function() {

@caub caub Sep 16, 2019

Copy link
Copy Markdown
Contributor Author

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.

cf gulpjs/undertaker#54 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@niftylettuce

niftylettuce commented Sep 17, 2019

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caub

caub commented Sep 17, 2019

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@niftylettuce

niftylettuce commented Sep 17, 2019

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caub

caub commented Sep 17, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caub

caub commented Sep 18, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

So what do you choose?

  • undo all gulp changes in this PR
  • keep it like this
  • drop completely gulp, use npm scripts as it simplifies the flow and gulp is not really necessary anyway
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@niftylettuce

niftylettuce commented Sep 18, 2019

Copy link
Copy Markdown
Contributor

npm scripts would be awesome
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@caub
caub force-pushed the patch-1 branch from 2d5ea93 to efb3913 Compare September 20, 2019 22:09
@caub

caub commented Sep 20, 2019

Copy link
Copy Markdown
Contributor Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
caub added 2 commits September 21, 2019 00:27
@caub
fix stacktracejs#18
147b7a5
@caub
add a test
b44fdc2
@caub
caub force-pushed the patch-1 branch from efb3913 to b44fdc2 Compare September 20, 2019 22:30
@eriwen
eriwen merged commit b44fdc2 into stacktracejs:master Jan 5, 2020
@eriwen

eriwen commented Jan 5, 2020

Copy link
Copy Markdown
Member

Thanks for your contribution, @caub. I now have some time to give stacktrace.js some love and have merged your PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eriwen eriwen Awaiting requested review from eriwen

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@eriwen eriwen

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@caub @eriwen @niftylettuce
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.