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

Examples: Update fflate version #21669

Merged
merged 6 commits into from
Apr 19, 2021
Merged

Examples: Update fflate version #21669

merged 6 commits into from
Apr 19, 2021

Conversation

gkjohnson
Copy link
Collaborator

Related to #21654

Description

See comment here.

This PR updates the fflate file in js/libs and jsm/libs to use the latest esm and umd files from the published package. The new esm variant does not use require so it shouldn't cause issues with skypack. I'm not sure where the previous minified fflate module came from but this one is not minified. Perhaps we should change the file name?

You can see that the new module version of fflate functions correctly in the fbx example:

https://raw.githack.com/gkjohnson/three.js/fflate-update/examples/webgl_loader_fbx.html

For some reason I'm not able to get the FBX example to work at all when switching it over to use the classic examples/js/ scripts even before updating fflate. I get an "unknown format" error -- am I missing a step here?

cc @Mugen87 @donmccurdy

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2021

@gkjohnson The issue in FBXLoader should be fixed via #21671. Do you mind trying again?

@marcofugaro It seems npm run build-examples formats strings in a way that potentially breaks code. In FBXLoader, the following line:

const CORRECT = 'Kaydara FBX Binary  \0'; 

was transformed to:

const CORRECT = 'Kaydara FBX Binary	\0'; 

Notice that the two spaces after "Binary" were replaced with a tab. This breaks the detection of binary FBX files.

I've patched this by using unicode characters which is more robust anyway. However, it's probably better to ensure npm run build-examples does not perform such changes to strings.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2021

I'm not sure where the previous minified fflate module came from but this one is not minified.

The developer of fflate provided these builds for us.

Perhaps we should change the file name?

Yes, otherwise it's confusing.

@gkjohnson
Copy link
Collaborator Author

  • Renamed fflate.module.min.js to fflate.module.js
  • Used find and replace to adjust fflate imports in jsm files
  • Tested FBX Loader example with classic script tag and module variants

Thanks @Mugen87! That fixed it

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2021

This line needs an update, too.

import { unzipSync } from './jsm/libs/fflate.module.min.js';

The editor also imports fflate in Loader.js, Menubar.File.js and sw.js.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2021

And here:

code = code.replace( 'import * as fflate from \'../libs/fflate.module.min.js\';', '' );

@gkjohnson
Copy link
Collaborator Author

ooh sneaky. I'd just checked in the /jsm folder. Last commit fixes those two lines.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 18, 2021

Unfortunately, the PR is still incomplete. Next to webgl2_rendertarget_texture2darray, please update:

import { unzipSync } from './jsm/libs/fflate.module.min.js';

import { unzipSync, strFromU8 } from '../../examples/jsm/libs/fflate.module.min.js';

import { zipSync, strToU8 } from '../../examples/jsm/libs/fflate.module.min.js';

'../examples/jsm/libs/fflate.module.min.js',

@marcofugaro
Copy link
Contributor

@marcofugaro It seems npm run build-examples formats strings in a way that potentially breaks code.

@Mugen87 oh yeah, it's because of this line:

code = code.replace( doubleSpaces, '\t' );

I saw your fix in #21671, thank you!

I'll see if we can make eslint change spaces to tabs with this rule: https://eslint.org/docs/rules/indent

@gkjohnson
Copy link
Collaborator Author

@Mugen87 sorry, thanks for the catch. Just updated and I no longer see any references to fflate.module.min.js in the entire repository.

@mrdoob mrdoob added this to the r128 milestone Apr 19, 2021
@mrdoob mrdoob merged commit 2b90507 into mrdoob:dev Apr 19, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 19, 2021

Thanks!

@gkjohnson gkjohnson deleted the fflate-update branch April 19, 2021 09:14
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