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

include es5 versions of individual source files in build #806

Closed
wants to merge 1 commit into from

Conversation

amagee
Copy link

@amagee amagee commented Aug 7, 2018

This pull request is basically a working proof of concept of this comment: #725 (comment)

I haven't thought through all the issues but hopefully this can get a discussion going.

@mcwhittemore
Copy link
Contributor

@amagee - Thanks for pushing forward for this via a PR. My main concern for a PR that changes the build system is ensuring that the result is non-breaking for other users (we've failed at this in the past). How would your propose testing this to make sure it works in many situations?

@amagee
Copy link
Author

amagee commented Aug 8, 2018

Yep good question.

  1. People using the published npm packages
    Assuming you run npm run build as part of your scripts to publish to npm, and the build succeeds, it should not be possible for this to break anything for anyone, since the main file generated by the build is unchanged; there are just new additional files. People who just import Mapbox Draw normally should be unaffected by the new files.

  2. People running builds themselves
    In its current form this could cause issues (eg. if run on a system that doesn't have sed, eg. Windows). This should be easily fixable though, if it's decided this is a sensible approach then I could replace the shell script with a Javascript file that does the same thing.

@jmking
Copy link

jmking commented Aug 29, 2018

Wondering if there is any more feedback on how we might proceed with this? (I work with @amagee)

@kkaefer
Copy link
Member

kkaefer commented Jan 7, 2020

Superseded by #945

@kkaefer kkaefer closed this Jan 7, 2020
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.

4 participants