Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

Ignore 'tags' when building #56

Merged
merged 2 commits into from
Dec 13, 2017
Merged

Ignore 'tags' when building #56

merged 2 commits into from
Dec 13, 2017

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Dec 12, 2017

'tags' file is generated by ctags and used to help navigating code to ease development. It was accidentally packed into 4.1.0 package by a person (me) who uses ctags. It should be removed from the package.

Related: zapier/zapier-platform-cli#165

P.S. For the longer term, I think it's best to use Travis CI to help us release production builds to avoid this kind of problem.

'tags' file is generated by ctags and used to help navigating code to ease
development. It was accidentally packed into 4.1.0 package and should be
removed.

Related: zapier/zapier-platform-cli#165
@xavdid
Copy link
Contributor

xavdid commented Dec 12, 2017

I'm missing a little context here - could you talk about what this builds? Does the core get shipped separately to lambda?

I'm a little concerned with how many exclusions we're adding. What happens if we add a hypothetical hashtags.js that's part of the lib. It would get skipped, right?

Maybe we should build inclusively instead? Explicitly add src, etc.

@eliangcs
Copy link
Member Author

eliangcs commented Dec 12, 2017

@xavdid Try npm install zapier-platform-core@4.1.0 and cd node_modules/zapier-platform-core. You'll see a 22MB file named 'tags'. It was not there until 4.1.0.

And I'm using regex pattern ^./tags$, so it should exclude ./tags and ./tags only. Other things like ./tags/123, ./123/tags and ./hashtags.js won't get skipped.

@xavdid
Copy link
Contributor

xavdid commented Dec 12, 2017

That's fair. This fixes the immediate problem, i'm just not sure how it helps the longterm issue of

  1. accidentally add something
  2. realize
  3. update build script
  4. push smaller version.

So yeah on the surface this is fine, it would just be cool to get in front of that ^

Copy link
Contributor

@bcooksey bcooksey left a comment

Choose a reason for hiding this comment

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

Whoops, def don't want to include that in the builds.

To @xavdid's point about avoiding this in the future, I think @eliangcs idea of travis would be a good one. We already have an account. Would it be tough to setup?

@xavdid
Copy link
Contributor

xavdid commented Dec 12, 2017

@eliangcs eliangcs merged commit 82cb1f6 into master Dec 13, 2017
@eliangcs eliangcs deleted the ignore-tags branch December 13, 2017 02:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants