Skip to content

Conversation

@Spacerat
Copy link
Contributor

This fixes a regression in the 4.0 release where building projects using JS concatenation would result in a

TypeError: expected a string or other character buffer object
error.

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-335 August 30, 2016 18:57 Inactive
ctx.set_group('bundle')
ctx.pbl_bundle(binaries=binaries, js=ctx.path.ant_glob(['src/pkjs/**/*.js', 'src/pkjs/**/*.json']), js_entry_file='src/pkjs/{{pkjs_entry}}')
"""
""".replace('{{pkjs_entry}}', project.pkjs_entry_point)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to just chain this with the other replace instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean, something like...

return wscript.replace('{{jshint}}', 'True' if jshint and not for_export else 'False').replace('{{pkjs_entry}}', project.pkjs_entry_point or '')

?
It was chained before, but the problem was that project.pkjs_entry_point can be None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify why this should always work:

  • pkjs_entry_point is not-None if the project is a package, rocky, or [native project with commonjs selected]
  • This function is only called for native projects, and the {{pkjs_entry}} replacement only happens with commonjs selected, so pkjs_entry_point will never be None.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, my concern was not with the jshint parameter, but rather the pkjs_entry one. However, given that pkjs_entry being None is no longer an issue, I think chaining the pkjs replace with the existing jshint replace is sufficient.

@cjwilliams
Copy link
Contributor

Would you mind fixing

ctx.pbl_worker(source=ctx.path.ant_glob(['worker_src/c/**/*.c', 'src/js/**/*.json']), target=worker_elf)
while you're at it? We shouldn't be passing JSON files into the compiler, that makes no sense

@Spacerat
Copy link
Contributor Author

Do you want to get rid of just that instance of passing in json, or all of them?

@cjwilliams
Copy link
Contributor

ctx.pbl_worker(source=ctx.path.ant_glob('worker_src/c/**/*.c'), target=worker_elf) instead of what's there now

@cjwilliams
Copy link
Contributor

👍

@Spacerat Spacerat merged commit 474b16b into master Aug 30, 2016
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