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

feat: add file operations in preview #936

Merged
merged 11 commits into from
Jan 24, 2019
Merged

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented Jan 15, 2019

preview

Closes #813.

@fsdiogo fsdiogo self-assigned this Jan 15, 2019
@ghost ghost added the status/in-progress In progress label Jan 15, 2019
@fsdiogo fsdiogo changed the title [WIP] feat: add file operations in preview feat: add file operations in preview Jan 16, 2019
Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM

@olizilla
Copy link
Member

@fsdiogo I'm not sure why we're seeing so many changes to the package-lock.json... can you talk me through your workflow? This is probably all super obvious, but I want to share steps we go through when devloping to see if there are differences that might explain it.

When trying out a branch i tend to just run:

git checkout <branch>
npm install
npm start

and that doesn't typically lead to a change in the package-lock.json (though it does on occasion, would be great to figure out why)

When adding a dep while working on a branch, i run

npm install <dep>

and that updates both the package.json and the package-lock.json. Typically it just updates the section of the package-lock.json that is relevant to the added dep, Somtimes it changes more of the package-lock.json than expected as the dep tree of the new lib causes some other deps to get hoisted / deduped in the tree.

In this PR, there is no change to the package.json so I wouldn't expect any change to the package-lock.json to be included in the PR changeset. Our locked dep tree should only be chaged when we decide to add things or delibreately want to update things.

At this moment, there are bunch of updates to ipld that we can't pull in until we do a bunch of work to update ipld-explorer-components. There are some other transitve deps in our tree that seem to regularly causes issues, something around level.js and leveldown. changes to them in the package-lock regularly cause build failures, so I'm being extra cautious about changes to the package-lock.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 17, 2019

... and that doesn't typically lead to a change in the package-lock.json (though it does on occasion, would be great to figure out why)

That is my workflow. Although when I check out to a new branch and install the deps the package-lock.json seems to be updated more often that not.

What happened in the last two commits was that I removed the package-lock.json and generated a new one to try to pass the CI, but some tests were failing, so I reverted it.

@olizilla
Copy link
Member

@fsdiogo I hear you. When the package-lock does change without me editing the package.json, i've either been resetting it with a git checkout -- package-lock.json if it's just in my local changeset, or finding the commit sha for where i branched off master and resetting the package-lock so it is removed from the changeset altogether with git checkout <sha from current master> package-lock.json.

The general pattern I'm following is, if the package.lock hasn't changed in the PR, then the packge-lock.json shouldn't be in the PR.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 17, 2019

I'll get the package-lock.json from master and this should be fixed!

Copy link
Member

@hacdias hacdias left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@olizilla
Copy link
Member

@fsdiogo this looks good, but there is a rendering issue in firefox

screenshot 2019-01-21 at 09 24 00

Please remember to check things in at least Chrome and Firefox! We need to wire up a service to automate the cross-browser testing, but in the meantime, it's on us.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Jan 23, 2019

Now rendering correctly in Firefox:

ff

Will merge when the CI is ✅

@fsdiogo fsdiogo merged commit 6926e40 into master Jan 24, 2019
@ghost ghost removed the status/in-progress In progress label Jan 24, 2019
@fsdiogo fsdiogo deleted the feat/add-file-ops-in-preview branch January 24, 2019 10:43
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

Successfully merging this pull request may close these issues.

3 participants