Merged
Conversation
c-harding
commented
Aug 11, 2018
| const contents = /\.gz$/i.test(name) ? await readGz(file) : await readFile(file); | ||
| name = name.replace(/\.gz$/i, ''); | ||
| if (/\.(gpx|tcx)$/i.test(name)) { | ||
| return await handleTrack(contents, file.name); |
Collaborator
Author
There was a problem hiding this comment.
This feels rather more convoluted than before, because:
- Reading the contents of an image as text doesn't make sense, so handling images should be done before unzipping.
- Gzipped files have a double extension, so
file.name.split('.').pop().toLowerCase()would not contain the information on the underlying file type. handleTrackshould not have to deal with the type of the file.handleFilecan't be called recursively, because wrapping the unzipped result back into a file object would be hard and inefficient.
Alternatively, handleFile could be rewritten to take the extension, as before, and either
- call
handleImage, - call
handleTextFilewith the contents of the unzipped file, as well as the remaining filename, or - call
handleTextFilewith the contents of the text file, as well as the remaining filename.
| } | ||
| const contents = /\.gz$/i.test(name) ? await readGz(file) : await readFile(file); | ||
| name = name.replace(/\.gz$/i, ''); | ||
| if (/\.(gpx|tcx)$/i.test(name)) { |
Collaborator
Author
There was a problem hiding this comment.
Is it worth storing whether the file is a TCX or a GPX file, or is it enough to look at what the root element within it is (src/track.js:21)?
Owner
There was a problem hiding this comment.
I think your implementation works fine for this 👍
erik
approved these changes
Aug 12, 2018
Owner
erik
left a comment
There was a problem hiding this comment.
This is great! Thank you for putting in the effort to get the new format working.
Works well for me locally so far.
| } | ||
| const contents = /\.gz$/i.test(name) ? await readGz(file) : await readFile(file); | ||
| name = name.replace(/\.gz$/i, ''); | ||
| if (/\.(gpx|tcx)$/i.test(name)) { |
Owner
There was a problem hiding this comment.
I think your implementation works fine for this 👍
erik
added a commit
that referenced
this pull request
Aug 12, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Following the new Strava GDPR update (#17), the export now includes the tracks in their original format. These are sometimes gzipped, and can be either GPX or TCX (common for MapMyRide transfer). This PR adds support for such files.
Minor changes:
npm runscripts for linting, building and serving, so that webpack, eslint and webpack-serve do not need to be installed globally.bundle.jsto root, rather thandist/, as it was copied back immediately in the build script. This way it can be used live with the dev server.