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

get version constant from package.json #5107

Merged
merged 4 commits into from
Mar 20, 2021

Conversation

micuat
Copy link
Member

@micuat micuat commented Mar 18, 2021

Resolves #2525 continuing from #5096

Changes:

add version constant taking from package.json reflecting @lmccart 's comment #5096 (review)
However the downside is that the whole package.json seems to be exposed to the minifined file. This is not a problem as the project is open-source, but I wonder if it causes other issues.

snippets from compiled bundle:

      262: [
        function(_dereq_, module, exports) {
          module.exports = {
            name: 'p5',
            repository: 'processing/p5.js',
            scripts: {
              grunt: 'grunt',
              build: 'grunt build',
              dev: 'grunt browserify:dev connect:yui watch:quick',
              docs: 'grunt yui',
              'docs:dev': 'grunt yui:dev',
              test: 'grunt',
              lint: 'grunt lint-no-fix',
              'lint:source': 'grunt lint-no-fix:source',
              'lint:samples': 'grunt lint-no-fix:samples',
              'lint:fix': 'grunt lint-fix',
              'contributors:add': 'all-contributors add',
              'contributors:generate': 'all-contributors generate',
              generate: 'all-contributors generate',
              release: 'grunt release-p5',
              prepublishOnly: 'grunt prerelease'
            },
            'lint-staged': {
              ignore: ['test/js/**/*.js'],
              'Gruntfile.js': 'eslint',
              'docs/preprocessor.js': 'eslint',
              'utils/**/*.js': 'eslint',
              'tasks/**/*.js': 'eslint',
              'test/**/*.js': 'eslint',
              'src/**/*.js': [
                'eslint',
                'node --require @babel/register ./utils/sample-linter.js'
              ]
            },
            version: '1.2.0',
            devDependencies: {
              '@babel/core': '^7.7.7',
              '@babel/preset-env': '^7.10.2',
              '@babel/register': '^7.7.7',
          /**
           * Version of this p5.js.
           * @property {String} VERSION
           * @final
           */
          var VERSION = _dereq_('../../package.json').version;

PR Checklist

@akshay-99
Copy link
Member

Including the package.json file into the built client-side library feels a bit wrong to me, even though as an opensource project we won't have any security implications from doing so. A few other options to consider would be:

  1. Using browserify to set the version number as a global variable like here. Browserify already has access to the version number (It currently uses it to add the top banner). It can then be set as a p5 constant as well.
  2. Using some grunt task like grunt-string-replace as seen in this answer to replace a pattern in the constants file with the version number

@limzykenneth
Copy link
Member

Is it possible to import the version number using ES6 import statement and let the build step tree shaking the unneeded info from package.json? Not sure if something like that will work at all though. If not then something like option 1 proposed by @akshay-99 would probably be fine as well.

@micuat
Copy link
Member Author

micuat commented Mar 20, 2021

thanks for the comments!
@limzykenneth My understanding is that it's only possible by separating the files based on dependency - for example version.json referred to by package.json so that the script requires version.json - which doesn't work in our case. So I chose option 1 ->
@akshay-99 thanks for the spot-on suggestion! I updated the branch to replace the code in grunt - I'm not sure if there's a "cleaner" way to achieve this.

@micuat
Copy link
Member Author

micuat commented Mar 20, 2021

oh. test failed because of the undefined const. maybe replacing this with a valid string e.g. 'VERSIONCONST' would be the solution?

@limzykenneth
Copy link
Member

I think you can ask the linter to ignore that line as well.

@outofambit
Copy link
Contributor

this is awesome @micuat! one tiny request, could we use something like 'VERSION_CONST_WILL_BE_REPLACED_BY_BROWSERIFY_BUILD_PROCESS' instead of 'VERSIONCONST' to help communicate how this string is changed in the build process? we don't need anything else before we merge this right?

@micuat
Copy link
Member Author

micuat commented Mar 20, 2021

@outofambit thanks <3 yes I totally agree, I also thought that the text should be unique + recognizable if something broke during the grunt process

@micuat micuat marked this pull request as ready for review March 20, 2021 20:02
@lmccart
Copy link
Member

lmccart commented Mar 20, 2021

awesome, thanks everyone!!

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.

Feature request: check p5.js "version" from code.
5 participants