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

Make updates for compatibility with Node 14, 16 #84

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

lognaturel
Copy link
Collaborator

@lognaturel lognaturel commented Jul 12, 2021

Closes #80, closes #72, closes #68, addresses short-term issue in #83

Builds with all tests passing on:

I tried to use https://github.com/renanccastro/libxmljs but compilation failed in my environment. I discovered https://github.com/MrMYHuang/libxmljs which included the changes I needed so I just pointed to that one's npm release.

@albanm
Copy link
Owner

albanm commented Jul 12, 2021

I didn't know about the engines attribute in npm packages, but maybe it would be a good idea to use it to express the node 14+ dependency.

@lognaturel lognaturel marked this pull request as draft July 12, 2021 19:53
@lognaturel
Copy link
Collaborator Author

Thanks for the tip, @albanm, did not know that either! Now that I've thought more about it, I'd rather include Node 12 if possible. I've marked the PR as draft to indicate I'll still be looking into that as well as the CI failure.

@lognaturel
Copy link
Collaborator Author

lognaturel commented Jul 12, 2021

I initially thought this didn't work with node 12.22.3. The build worked fine but when running tests, I got Error: Module did not self-register: '<path>/node-libxslt/node_modules/node1-libxmljsmt-myh/build/Release/xmljs-myh.node'.

If I npm rebuild node1-libxmljsmt-myh --build-from-source first, then tests pass as expected. Same thing with node 10.24.1. I don't know enough about the tooling to understand what's going on or whether any action needs to be taken. My best guess is that the npm release has macOS binaries built with node 14+ which I pull when building locally with node 12 but then linking fails. Or maybe it's really just that I had previously built with node 14+ locally and that was being used. Any insights appreciated.

From @eyelidlessness:

When you tested node versions locally, did you wipe out node_modules and reinstall for each version? Generally anything with native bindings needs to be rebuilt for each version, and that would go for subdeps too

That makes sense and means that I've verified with node 12.22.3 and 10.24.1 as well. I think this can be released as 0.9.1.

The CI failure was because I had failed to make a Windows-specific change to use the myh fork here. I forced push to get that in and I've left the CI node version at 12 for now.

@lognaturel lognaturel marked this pull request as ready for review July 12, 2021 21:46
@albanm
Copy link
Owner

albanm commented Jul 12, 2021

I don't know why your tests failed in this particular case, but it looks like appveyor build is ok with node 12 so I am not worried.

I will have a closer look soon and certainly merge then release it too.

@albanm albanm merged commit df39004 into albanm:master Jul 16, 2021
@albanm
Copy link
Owner

albanm commented Jul 16, 2021

Merged and released. Thanks !

@lognaturel lognaturel deleted the node-16 branch July 16, 2021 15:37
@lognaturel
Copy link
Collaborator Author

Please note that this does not currently work with NPM 7. Filed issue at npm/cli#3556

@vckeating
Copy link

vckeating commented Aug 10, 2021

I initially thought this didn't work with node 12.22.3. The build worked fine but when running tests, I got Error: Module did not self-register: '<path>/node-libxslt/node_modules/node1-libxmljsmt-myh/build/Release/xmljs-myh.node'.

If I npm rebuild node1-libxmljsmt-myh --build-from-source first, then tests pass as expected. Same thing with node 10.24.1. I don't know enough about the tooling to understand what's going on or whether any action needs to be taken. My best guess is that the npm release has macOS binaries built with node 14+ which I pull when building locally with node 12 but then linking fails. Or maybe it's really just that I had previously built with node 14+ locally and that was being used. Any insights appreciated.

From @eyelidlessness:

When you tested node versions locally, did you wipe out node_modules and reinstall for each version? Generally anything with native bindings needs to be rebuilt for each version, and that would go for subdeps too

That makes sense and means that I've verified with node 12.22.3 and 10.24.1 as well. I think this can be released as 0.9.1.

The CI failure was because I had failed to make a Windows-specific change to use the myh fork here. I forced push to get that in and I've left the CI node version at 12 for now.

After pulling in the recent version update, I am able to build my application locally using the version 0.10.0. https://www.npmjs.com/package/libxslt
However, I run into this error when I deploy to my AWS environment which is linux based. Error: Module did not self-register: 'var/task/node_modules/node1-libxmljsmt-myh/build/Release/xmljs-myh.node'.
Additional note, I am using yarn and not npm.

I am not sure if this change means I can no longer use this package for a deployment to a linux environment as oppose to windows and wanted to ask. In addtion, I am currently using Node 14 which may be part of the issue since I see here that this updagrade is set for Node 12 and I am guessing creates a compliance issue. I have been trying to stay away from using Node 12 since this version is already in maintenance mode. https://nodejs.org/en/about/releases/ Is there an intention to continue with upgrading to be node 14 compliant?

@eyelidlessness
Copy link

@vckeating This change was intended to support Node 14 and 16. I have PRs updating several of our projects to this version, with CI matrix tests of 14 and 16 on Linux (Ubuntu Xenial in some projects due to an oversight on my part, Focal on another).

I have also validated installation with Yarn 1.x on my local Mac while investigating potential solutions to the issues we encountered with NPM 7. I have not tried Yarn in our CI environments. Not sure if either of these may be issues, but for more detail in case this helps track down the problem:

  1. Are you using Yarn 2?
  2. Are you using Yarn PnP?

And in case the deeply nested quote slipped by, are you upgrading an existing environment, and if so did you try a clean install?

@vckeating
Copy link

The local version of yarn is v1.22.5 which is what is used to build the yarn.lock file.
Also, the top of the yarn.lock file indicates:

yarn lockfile v1

and the deployment using yarn v1.22.11

Local is built in a Windows development enviroinment.
During deployment the yarn.lock file us used to build the dependancies needed for the application. The node_modules are ignored by git so that the required details of the dependancy are taken from the yarn lock..
The exact command used is:

yarn install --frozen-lockfile

I have also tried this same libexslt v0.10.0 with Node 12 and it results in the same error when invoking the lambda after AWS deployment.

When building locally I am deleting the node_modules folder and the yarn lock beforeI make the update to the node version and libxslt. Then once they are deleted I run:

yarn install

The changes commited are the package.json for the libxslt update and
Bamboo specs is utilized for the build and deployment of the application.

I do not believe this is using Yarn Pnp, because there are no .pnp or .cjs files and it is clear that the yarn.lock is designating v1.

@vckeating
Copy link

vckeating commented Aug 18, 2021

Update, the issue with the error was resolved with 2 additional changes in my case.

In the package.json file I updated the script for "build" to the below:
"build": "tsc --build && yarn package-lambda-<Name>
"package-lambda-<Name>": "cd ./src/transformation && yarn install --production && cd -"

Without the above in the scrips the node_modules folder was left off the deployment and I recieved a different error when invoking the Lambda. I have seen alternatives where webpack is used but I did not test that.

In addition, there were some additional configuration changes that I had to make to Bamboo Specs v7.
Those settings changes specified the version of node, npm, and yarn by looking at what was indicated for "engines" in the package.json

Hope this is helpful to others who encounter this error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants